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

builtin: fix map.clear() not resetting map's metas and keys blocks (fix #22139) #22140

Merged
merged 4 commits into from
Sep 1, 2024

Conversation

spytheman
Copy link
Member

@spytheman spytheman commented Aug 31, 2024

Fix #22139 .

@ka-weihe
Copy link
Member

Hi @spytheman 😄

I think you also have to clear/free the memory of the denseArray. I think that right now it will just leak.

@spytheman
Copy link
Member Author

spytheman commented Aug 31, 2024

@ka-weihe thanks for reviewing.

I think, that the explicit goal of that .clear() method, rather than just m = {}, was to not free the main memory blocks, used for the keys and values, so that they would be available right away, for reuse, in subsequent m[key]=val operations.

Is there a mechanism, where the old blocks would just leak (rather than being reused), and new ones will be allocated instead, when the user does this for example:

mut m := map[int]int{}
m[123] = 456
m[789] = 321
m.clear()
m[1] = 10
m[2] = 20

@ka-weihe
Copy link
Member

Is there a mechanism, where the old blocks would just leak (rather than being reused), and new ones will be allocated instead, when the user does this for example:

mut m := map[int]int{}
m[123] = 456
m[789] = 321
m.clear()
m[1] = 10
m[2] = 20

Hmm, after looking at the code again, I think it might actually be fine, but it would be interesting to run it in a loop and see if the memory usage keeps increasing. I don't think it will, but it's a good test.

@JalonSolov
Copy link
Contributor

Yes, the .clear() was to mark the map as "empty", but not change the amount of memory already allocated. Too much memory churn that way.

You can always do m = map[int]int{} again to completely wipe it, free the memory, and start over. Or just m = {} if that works.

@spytheman
Copy link
Member Author

Hmm, after looking at the code again, I think it might actually be fine, but it would be interesting to run it in a loop and see if the memory usage keeps increasing. I don't think it will, but it's a good test.

I've added cmd/tools/bench/map_clear.v and cmd/tools/bench/map_clear_runner.vsh to make it easier to test that, for different options and iteration counts.
The results is that there is no increase in the memory usage, for me on Ubuntu 20.04:
image

I've tested with these commands on my latest commit, if someone else wants to check:
FLAGS='' cmd/tools/bench/map_clear_runner.vsh 1_000_000 100_000_000 10_000_000
FLAGS='-gc none' cmd/tools/bench/map_clear_runner.vsh 1_000_000 100_000_000 10_000_000
FLAGS='-prod -cc clang' cmd/tools/bench/map_clear_runner.vsh
FLAGS='-gc none -prod -cc clang' cmd/tools/bench/map_clear_runner.vsh

(you will need the latest commit 63a51ff)

@spytheman
Copy link
Member Author

Here is a heaptrack view after running:
v -gc none -cc gcc -cg -keepc cmd/tools/bench/map_clear.v
heaptrack cmd/tools/bench/map_clear 50_000_000 (the run takes ~4.6 seconds)
heaptrack --analyze "/space/v/oo/heaptrack.map_clear.87529.zst"
image

@spytheman
Copy link
Member Author

And the same, but after a longer run heaptrack cmd/tools/bench/map_clear 99_000_000 (takes 8.8s):
image

@spytheman spytheman merged commit 673ac0a into vlang:master Sep 1, 2024
75 checks passed
@spytheman spytheman deleted the fix_issue_22139_with_map_clear branch September 1, 2024 09:02
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.

Can't add same element to cleared map
3 participants