packedsets: simplify implementation using Table#1687
Draft
alaviss wants to merge 4 commits intonim-works:develfrom
Draft
packedsets: simplify implementation using Table#1687alaviss wants to merge 4 commits intonim-works:develfrom
alaviss wants to merge 4 commits intonim-works:develfrom
Conversation
The new implementation pack ordinals by clustering their least-significant byte in a bitset. This optimizes the set for the common case of checking in sequential keys.
The default equivalence check does not come through sometimes, so implement our own. Also fix wrong set comparison operator used for comparing words.
7110437 to
9880595
Compare
I forgot to preserve the elements in the LHS...
They already doesn't, but add some try-except so the compiler doesn't complain.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Instead of using a custom ordered table implementation, convert
packedsetsto take advantage ofTableand built-inset[T]. This cut down on the amount of code in the stdlib without compromising performance.Details
This came out of the prior work on Roaring Bitmap, where I needed a few sample implementations to measure performance with. I found the original
PackedSetresembles a simple ordered table with clustered lower bits, and experimented with building a simplified version based on existing stdlib components.In this new structure, keys are grouped by their top
N - 8bits and indexed via aTable, with the bottom 8 bits packed into aset[uint8]. Due to the change in internal structure,items()no longer yield keys in insert order.Performance metrics for
nim c --hints:off --warnings:off -fc compiler/nim.nim(on the same compiler checkout):For the compiler, the new implementation performance is very similar to the original.
Notes for Reviewers
While tests passes, the original suite isn't very comprehensive.Other compiler failures showed that the tests are not good enough. Draft until tests and microbenchs are written.inclandcontainswhile not employing many mathematical operations.