WIP: Sketch primops-based atomic counter and distribution implementation #42
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TravisWhitaker
force-pushed
the
memory-safe
branch
from
8288724 to
4273d93
Compare
TravisWhitaker
changed the title
TravisWhitaker
force-pushed
the
memory-safe
branch
from
4273d93 to
420f559
Compare
|
Turns out I mistakenly used an atomic write to release the distribution spinlock, when a weak write is sufficient (and much faster). Now the primops implementation is comparable to the existing implementation's performance (even a bit faster in the multicapability case). |
|
One way or another, #41 needs to be fixed for EKG to work well on newer ARM boards. If there's no interest in this patch, we should find another way to achieve memory safety on weakly ordered machines. |
awjchen
added a commit
to hasura/ekg-core
that referenced
this pull request
May 29, 2021
This commit attempts to address issue haskell-github-trust#41 of tibbe/ekg-core by replacing the C code for the distribution metric with GHC prim ops. The performance of this implementation is about half that of the existing C code in a single-threaded benchmark; without masking the performance is comparable. This commit is based on the work of Travis Whitaker in PR haskell-github-trust#42 of tibbe/ekg-core.
awjchen
added a commit
to hasura/ekg-core
that referenced
this pull request
Jun 23, 2021
This commit attempts to address issue haskell-github-trust#41 of tibbe/ekg-core by replacing the C code for the distribution metric with GHC prim ops. The performance of this implementation is about half that of the existing C code in a single-threaded benchmark; without masking the performance is comparable. This commit is based on the work of Travis Whitaker in PR haskell-github-trust#42 of tibbe/ekg-core.
|
@23Skidoo What more can I do to help get this package fixed on aarch64? |
AndreasPK
reviewed
Jun 19, 2023
| , cSum :: !Double | ||
| , cMin :: !Double | ||
| , cMax :: !Double | ||
| , cLock :: !Int64 -- ^ 0 - unlocked, 1 - locked |
There was a problem hiding this comment.
Seems like a good idea to keep this comment somehow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After finding #41 I was curious whether or not a primops-based implementation of cbits could achieve competitive performance. Turns out that it can, with some caveats.
I took four measurements to compare the implementation presented here with the current tip of master:
counterbenchmark with-N1counterbenchmark with-Ndistributionbenchmark with-N1distributionbenchmark with-NThe results were recorded on my 2018 i9 MacBook Pro, which has 12 logical cores (6 physical cores, two hyperthreads each), so
-Nmeans-N12on this machine. I added-O2toghc-optionsfor the current tip of master and the code here. These are the results for the current tip of master:And here are the results for the code presented here:
The new implementation yields slightly faster atomic counter performance in both the single-capability and capability-per-core cases. This could be due to the fact that in the new implementation the counter is not in pinned memory, or the lack of
withForeignPtr, which isn't free.The distribution results are quite interesting. In the single-capability case, this implementation is about three times slower than the existing implementation, but the gap between them almost entirely disappears in the capability-per-core case. I'm not quite sure what's going on here. I thought that perhaps the fact that the unsafe FFI call can't be interrupted by GC could have something to do with it, but using
-Sshows that only a single major GC at the end of benchmark execution takes place in both the single-capability case and the capability-per-core case. I suspect the answer is somewhere in the core emitted forspinLock, but I haven't dug into it yet.The most important difference between the two implementations is correctness. Because of #41, ekg sometimes reports stale or gibberish metrics on newer aarch64 chips, while this implementation produces correct results (at least the results look right to me, but a second pair of eyes on the distribution update code would be much appreciated).
I'm not too happy about having to unpack
I#from a CAF to get the field array offsets in these functions, but sincehsc2hsseems to always wrap parens around the results of its directives, I couldn't figure out how to generate an unboxed literal from them. One solution might be to export TH splices from a separate hsc file, then splice in the unboxed literals in the System.Metrics.Distribution module. That would also allow us to skip hsc2hs on that module entirely, which would roughly halve the number of#required.Another minor difference is the change from
Int64toInt. This is necessary because GHC doesn't make available any 64 bit wide atomic primops on 32 bit chips.