For the new value-tracking script cache, we can keep using the cuckoo
cache system but now using it as a map.
Details
Details
- Reviewers
markblundeberg - Group Reviewers
Restricted Project - Commits
- rSTAGING405c4da10609: allow cuckoocache to function as a map
rABC405c4da10609: allow cuckoocache to function as a map
make check
Diff Detail
Diff Detail
- Repository
- rABC Bitcoin ABC
- Branch
- arcpatch-D4621
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 8475 Build 14961: Default Diff Build & Tests Build 14960: arc lint + arc unit
Event Timeline
| src/test/cuckoocache_tests.cpp | ||
|---|---|---|
| 48 ↗ | (On Diff #14590) | 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. |
Comment Actions
You probably want to investigate why the build failed.
| src/cuckoocache.h | ||
|---|---|---|
| 434 ↗ | (On Diff #14590) | I think this comment adds more to the confusion than anything else. |
| 522 ↗ | (On Diff #14590) | That comment in itself is a strong tel there is some factoring that needs to takes place. |
| src/test/cuckoocache_tests.cpp | ||
| 31 ↗ | (On Diff #14590) | Don't copy bad style. Fix it instead. |
| 61 ↗ | (On Diff #14590) | 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. |
| 62 ↗ | (On Diff #14590) | 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. |
| 65 ↗ | (On Diff #14590) | 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? |
| 70 ↗ | (On Diff #14590) | 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. |
| 464 ↗ | (On Diff #14590) | 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. |
Comment Actions
Address comments, except making the constructor of the test map element explicit, as it makes the test way more complex and the upside is not very clear.
Comment Actions
| src/cuckoocache.h | ||
|---|---|---|
| 519 ↗ | (On Diff #14716) | Because it is completely pointless in a template. |
Comment Actions
green with some comments
| src/cuckoocache.h | ||
|---|---|---|
| 512 | just noticed this, seems to be missed from previous diff | |
| src/test/cuckoocache_tests.cpp | ||
| 107 | note that (since not re-seeded) this does 400000 calls. | |
| 501 | It's interesting that this manages to have 100000 successive inserts. Without any erases, the epoch system does allow the table to get pretty full. I tried setting this to 2 million iterations and it only fails 7 times. | |
| src/test/cuckoocache_tests.cpp | ||
|---|---|---|
| 501 | With each element having 8 positions, it's not that incredible. | |