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

map: make DenseArray::has_index not assume string keys #7284

Merged
merged 2 commits into from
Dec 13, 2020

Conversation

ntrel
Copy link
Contributor

@ntrel ntrel commented Dec 12, 2020

Part of #6991.

Add separate is_deleted array - this is only allocated the first time a key is deleted. This is allocated separately from the data array for better performance when there are no deleted elements (a common case).
Also set array cap in test.

Add separate `is_deleted` array - this is only allocated the first time
a key is deleted.
Also set array cap in test.
@@ -484,7 +497,6 @@ pub fn (mut m map) delete(key string) {
if m.key_values.deletes >= (m.key_values.len >> 1) {
m.key_values.zeros_to_end()
m.rehash()
m.key_values.deletes = 0
Copy link
Contributor Author

@ntrel ntrel Dec 12, 2020

Choose a reason for hiding this comment

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

Note: zeros_to_end already does this, so this was redundant.

vlib/builtin/map.v Outdated Show resolved Hide resolved
@ntrel ntrel marked this pull request as ready for review December 12, 2020 11:44
@medvednikov medvednikov requested a review from ka-weihe December 12, 2020 22:08
@ka-weihe
Copy link
Member

ka-weihe commented Dec 12, 2020

LGTM, but I think we need better tests (fuzzing) to ensure that we are not introducing bugs. We need something like https://github.com/ka-weihe/hashmap-v/blob/master/monkeytest.v, but better. I used to run that for multiple hours in multiple threads with -fsantize=memory and -fsanitize=undefined on.

@ka-weihe ka-weihe mentioned this pull request Dec 13, 2020
@ka-weihe
Copy link
Member

@ntrel I have added a fuzzer in #7297. Feel free to add more tests to it if it gets merged. I have not added it to CI, because it is meant to be run for long periods on many cores. We could maybe add it to periodic?

@medvednikov medvednikov merged commit ae460a2 into vlang:master Dec 13, 2020
@ntrel ntrel deleted the map-deleted branch December 14, 2020 10:52
@ntrel
Copy link
Contributor Author

ntrel commented Dec 14, 2020

@ka-weihe OK, thanks.

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.

4 participants