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

Hashtable #47

Merged
merged 3 commits into from
Sep 11, 2024
Merged

Hashtable #47

merged 3 commits into from
Sep 11, 2024

Conversation

vishalgupta97
Copy link
Contributor

Depends on #46

@vishalgupta97 vishalgupta97 requested a review from mjp41 September 2, 2024 13:38
Copy link
Member

@mjp41 mjp41 left a 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.

* single writer is running at a given point in time.
*/

#define DEBUG_RW 0
Copy link
Member

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();

Copy link
Member

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.

Suggested change
xoroshiro::p128r32 rand{Systematic::get_prng_next()};


for (size_t i = 0; i < num_operations; i++)
{
size_t dependent_buckets = (rand() % num_dependent_buckets) + 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
size_t dependent_buckets = (rand() % num_dependent_buckets) + 1;
size_t dependent_buckets = (rand.next() % num_dependent_buckets) + 1;


for (size_t j = 0; j < dependent_buckets; j++)
{
size_t key = rand() % (num_buckets * num_entries_per_bucket * 2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
size_t key = rand() % (num_buckets * num_entries_per_bucket * 2);
size_t key = rand.next() % (num_buckets * num_entries_per_bucket * 2);

{
size_t key = rand() % (num_buckets * num_entries_per_bucket * 2);
size_t idx = key % num_buckets;
if (rand() % rw_ratio_denom < rw_ratio)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (rand() % rw_ratio_denom < rw_ratio)
if (rand.next() % rw_ratio_denom < rw_ratio)

Copy link
Member

@mjp41 mjp41 left a 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.

@mjp41 mjp41 enabled auto-merge (squash) September 11, 2024 13:47
@mjp41 mjp41 merged commit 721d697 into microsoft:main Sep 11, 2024
8 checks passed
vishalgupta97 added a commit to vishalgupta97/verona-rt that referenced this pull request Sep 16, 2024
* Add hashtable benchmark

* Minor changes from Code Review.

* Add systematic testing runs.

---------

Co-authored-by: Matthew Parkinson <[email protected]>
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.

2 participants