-
Notifications
You must be signed in to change notification settings - Fork 16
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
Hashtable #47
Hashtable #47
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, couple of minor things.
test/perf/hashtable/hashtable.cc
Outdated
* single writer is running at a given point in time. | ||
*/ | ||
|
||
#define DEBUG_RW 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this controllable from CMake? Then we can build two versions of this file?
void test_hash_table() | ||
{ | ||
auto t1 = high_resolution_clock::now(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use our PRNG, this can be seeded based on systematic testing, and will be cross platform debuggable. The standard library is free to define its randomness for a platform, which makes cross platform systematic testing hard.
xoroshiro::p128r32 rand{Systematic::get_prng_next()}; | |
test/perf/hashtable/hashtable.cc
Outdated
|
||
for (size_t i = 0; i < num_operations; i++) | ||
{ | ||
size_t dependent_buckets = (rand() % num_dependent_buckets) + 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t dependent_buckets = (rand() % num_dependent_buckets) + 1; | |
size_t dependent_buckets = (rand.next() % num_dependent_buckets) + 1; |
test/perf/hashtable/hashtable.cc
Outdated
|
||
for (size_t j = 0; j < dependent_buckets; j++) | ||
{ | ||
size_t key = rand() % (num_buckets * num_entries_per_bucket * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size_t key = rand() % (num_buckets * num_entries_per_bucket * 2); | |
size_t key = rand.next() % (num_buckets * num_entries_per_bucket * 2); |
test/perf/hashtable/hashtable.cc
Outdated
{ | ||
size_t key = rand() % (num_buckets * num_entries_per_bucket * 2); | ||
size_t idx = key % num_buckets; | ||
if (rand() % rw_ratio_denom < rw_ratio) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (rand() % rw_ratio_denom < rw_ratio) | |
if (rand.next() % rw_ratio_denom < rw_ratio) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, couple of minor things.
* Add hashtable benchmark * Minor changes from Code Review. * Add systematic testing runs. --------- Co-authored-by: Matthew Parkinson <[email protected]>
Depends on #46