-
Notifications
You must be signed in to change notification settings - Fork 772
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
Set: Optimize away isinst
check
#10860
Conversation
Store height in leaves. Compared to the old discussion, when Left/Right were proposed to be stored in a universal node, this adds 4 bytes to leaves or 2 bytes per item on average (vs 16/8).
`Match` produces `sub 1` and `switch` instruction. Here, for any non-trivial count, nodes are more frequent than leaves on the path, so branch prediction should be beneficial.
@buybackoff No worries, we'll figure out the right merge strategy. I believe that because the changes are the same, it shouldn't matter if we take them independently. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good for set too (I mean, they're the same, but hey, looks good!)
@dsyme if you could take a look here as well that would be great. @buybackoff just FYI - in case this does take time to merge - we have a policy of two approvals required for PRs. We relax that a bit when the changes are straightforward and/or tiny, but this will require two sign-offs to proceed since it affects such a core set of data structures. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thanks for this.
Same as #10845
If OK, this PR includes the Map changes. The Map one could be closed and this merged, or will rebase.