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

LSM,VSR: Backport std HashMap fix #867

Merged
merged 3 commits into from
Jun 14, 2023

Conversation

sentientwaffle
Copy link
Member

In zig 0.9.1 (and zig master, as of writing this) fetchRemove does not increment available.
So it leaks slots.
Currently we only use fetchRemove in the mutable table and the client sessions.

Note that the VOPR didn't find this bug in client sessions because evictions are not tested.
It probably didn't find it in the mutable table because we often use createRetainingCapacity(), which restores the leaked slots.

Backport the fix: ziglang/zig#15989.

In zig 0.9.1 (and zig master, as of writing this) `fetchRemove` does not increment `available`.
So it leaks slots.
Currently we only use `fetchRemove` in the mutable table and the client sessions.

Note that the VOPR didn't find this bug in client sessions because evictions are not tested.
It probably didn't find it in the mutable table because we often use `createRetainingCapacity()`, which restores the leaked slots.

Backport the fix: ziglang/zig#15989.
@sentientwaffle sentientwaffle marked this pull request as ready for review June 9, 2023 20:51
Copy link
Member

@jorangreef jorangreef left a comment

Choose a reason for hiding this comment

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

Do we need to update a few other uses of std HashMap?

For example, (posted_)groove.zig, superblock_manifest.zig, auditor.zig and message_bus.zig?

src/lsm/table_mutable.zig Outdated Show resolved Hide resolved
@sentientwaffle
Copy link
Member Author

For example, (posted_)groove.zig, superblock_manifest.zig, auditor.zig and message_bus.zig?

The backported hashmap is only needed when we use fetchRemove. So only the mutable table's value cache and the client sessions hashmap need to be updated.

@sentientwaffle sentientwaffle added this pull request to the merge queue Jun 14, 2023
Merged via the queue into main with commit 8bb8768 Jun 14, 2023
110 of 112 checks passed
@sentientwaffle sentientwaffle deleted the dj-hash-map-fetch-remove-workaround branch June 14, 2023 09:54
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.

None yet

3 participants