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.

Sign up for GitHub

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

Jump to bottom

Improve perf of Dict and Set #6176

Merged
merged 11 commits into from
Dec 6, 2023
Merged

Improve perf of Dict and Set #6176

merged 11 commits into from
Dec 6, 2023

Conversation

bhansconnect
Copy link
Contributor

Misc changes to improve Dict and Set performance.

rtfeldman
rtfeldman previously approved these changes Dec 4, 2023
View reviewed changes
@bhansconnect
Copy link
Contributor Author

I updated the Dict.keepShared semantics because it was not very clear to begin with and turns out to be bad for perf. Since it would block my new more performant impl, I just changed the semantics. I think this is clearer anyway.

Now for dict, keepShared only returns elements where both the key and value match. So they are 100% shared.

@bhansconnect
Copy link
Contributor Author

Random unrelated valgrind tests where failing. Rebased in hopes the errors will go away.

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 5, 2023

Note that the macos-x86-64 tests are not using the macos x86 workflow, they're using the ubuntu x86 one. That was an accidental copy-paste error that's fixed in #6183.

@rtfeldman
Copy link
Contributor

Still seeing the valgrind errors unfortunately...

@Anton-4
Copy link
Collaborator

Anton-4 commented Dec 5, 2023 •
edited
Loading

Seems likely like something similar is going on to what John hit here. There it was list_concat_empty_list_zero_sized_type and now it is list_concat_consumes_second_argument. Old discussion of fix.

@bhansconnect
Copy link
Contributor Author

I think rebasing on main after merging #6062 fixed the valgrind issue. Now just some wasm issue...hmm. Proabably a miscompilation on the wasm dev backend?

@bhansconnect
Copy link
Contributor Author

I decided to just work around the wasm bug. I just rewrote keepShared to have the exact same semantics but sligthly different code gen to get around the bug.

Anyway, I think this works and should pass CI now.

@bhansconnect bhansconnect merged commit 81eff6a into main Dec 6, 2023
17 checks passed
@bhansconnect bhansconnect deleted the set-perf branch December 6, 2023 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Anton-4 Anton-4 Anton-4 approved these changes

@rtfeldman rtfeldman Awaiting requested review from rtfeldman

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bhansconnect @Anton-4 @rtfeldman