Page MenuHomePhabricator

allow cuckoocache to function as a map
Needs RevisionPublic

Authored by markblundeberg on Tue, Dec 3, 05:49.


Group Reviewers
Restricted Project

For the new value-tracking script cache, we can keep using the cuckoo
cache system but now using it as a map.

Test Plan

make check

Diff Detail

rABC Bitcoin ABC
Lint OK
No Unit Test Coverage
Build Status
Buildable 8385
Build 14788: Bitcoin ABC Buildbot
Build 14787: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Tue, Dec 3, 05:49
Herald added a reviewer: Restricted Project. · View Herald TranscriptTue, Dec 3, 05:49
markblundeberg added inline comments.Tue, Dec 3, 17:28

Amusingly, if we just use u=0 for the last hash, all the tests pass fine. In the cuckoocache_hit_rate_ok test, this causes the normalized hit rate to drop from 0.989480 to 0.989442, compared to the sigcache's 0.989503. That is basically insignificant though, since changing the test seed makes much larger fluctuations.

(In contrast if I zero out the last *two* hashes then it causes tests to fail.)

In other words, we can probably drop to having the cache look at 7 hashes instead of 8. This increases the speed of a read miss by 14%, and the speed of a low-load insertion should see a similar boost since the initial search will be faster.

deadalnix requested changes to this revision.Tue, Dec 3, 17:41

You probably want to investigate why the build failed.


I think this comment adds more to the confusion than anything else.


That comment in itself is a strong tel there is some factoring that needs to takes place.


Don't copy bad style. Fix it instead.


If this is a KV element, then you really want to pass a key and a value to this instead of filling the value with random garbage.


You can then remove the setter. In general, when you end up with both setters (OO style) and getters (functional style) in the same object, you are doing something wrong.

If generating variant is still desired, you can have functions such as withValue(...) that generate and return a modified copy.


I would advice against the use of memcpy. Why not have an std::array<uint8_t, 28> and a uint32_t as member for the element?


Maybe it is a good idea to actually use a function such as matchKey(const ExampleMapElement &rhs) rather than use operator== because this is clearly not an equality test.


Use {} as an empty statement instead.

Also, I don't think this is a bit strange and I'm not sure what it is testing. This is potentially looping forever and not really testing that the value we get are working properly, because they are set randomly to begin with, see comments on ExampleMapElement.

This revision now requires changes to proceed.Tue, Dec 3, 17:41