Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrote s3-fifo inspired hash consing, to reduce locking when hitting the get #202

Closed
wants to merge 1 commit into from

Conversation

DavyLandman
Copy link
Member

Profiles showed that 5--10% was spend in Caffeines drain scheduler.

Now we accept a bit different behavior of the hotEntries (wrt to WeakReferenceHashConsingMap), and keep the performance more predictable.

Side-effect is that we have to make a better guess of the amount of entries to keep hot, as too big and we will be penalized for the linear scans, to small and we'll be paying for the reference wrapper.

…he get.

Profiles showed that 5--10% was spend in Caffeines drain scheduler.

Now we accept a bit different behavior of the hotEntries, and keep the performance more predicatable.

Side-effect is that we have to make a better guess of the amount of entries to keep hot
@DavyLandman
Copy link
Member Author

DavyLandman commented Sep 19, 2023

Note, it will require some benchmarking to see if it has the desired effect, especially due to the simple loop that ends up calling equals a lot more than an map would do. In my own benchmarks for just this class, it was quite a bit faster.

Alternatives to consider:

  1. store the hashcode in the FifoEntry class and compare against that
  2. iterate over the Fifo queues in the order (so use the indexes and mod with the size in a loop) instead of just iterating over the list
  3. implement simple direct probing map without collision nodes for hot entries

@github-actions
Copy link

Test Results

         96 files  ±    0           96 suites  ±0   26m 1s ⏱️ + 15m 59s
225 735 tests +  52  225 734 ✔️ +  52  1 💤 ±0  0 ±0 
677 292 runs  +156  677 289 ✔️ +156  3 💤 ±0  0 ±0 

Results for commit 47cb5d1. ± Comparison against base commit 39a8d25.

This pull request removes 52 and adds 104 tests. Note that renamed tests count towards both.
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, WeakReferenceHashConsingMap)[1]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, WeakReferenceHashConsingMap)[2]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, WeakReferenceHashConsingMap)[3]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, WeakReferenceHashConsingMap)[4]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ insertMultipleCopiesAllReturnSameInstance(int, int, WeakReferenceHashConsingMap)[1]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ insertMultipleCopiesAllReturnSameInstance(int, int, WeakReferenceHashConsingMap)[2]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ insertMultipleCopiesAllReturnSameInstance(int, int, WeakReferenceHashConsingMap)[3]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ insertMultipleCopiesAllReturnSameInstance(int, int, WeakReferenceHashConsingMap)[4]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ looseReference(int, int, WeakReferenceHashConsingMap)[1]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ looseReference(int, int, WeakReferenceHashConsingMap)[2]
…
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, HashConsingMap)[1]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, HashConsingMap)[2]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, HashConsingMap)[3]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, HashConsingMap)[4]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, HashConsingMap)[5]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, HashConsingMap)[6]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, HashConsingMap)[7]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ clearMostAndQueryRest(int, int, HashConsingMap)[8]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ insertMultipleCopiesAllReturnSameInstance(int, int, HashConsingMap)[1]
io.usethesource.vallang.util.WeakHashConsingMapTest ‑ insertMultipleCopiesAllReturnSameInstance(int, int, HashConsingMap)[2]
…

@DavyLandman
Copy link
Member Author

Ok, a few benchmarks revealed that this is not the right solution. s3-fifo paper is nice, but not for this specific problem and workload.

@DavyLandman DavyLandman deleted the s3-fifo-inspired-hash-consing branch September 20, 2023 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant