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.
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
implemented SparseIntSet #533
implemented SparseIntSet #533
Changes from 11 commits
9efb335
c38959c
4fb0485
4050d46
070b4f1
e228fad
61bac31
6f8555e
8254a23
23ddc6c
f248297
88fa2d8
77963a8
aec600d
e9377eb
43e436b
cabcc66
b91b8fb
523da53
29aaefa
9770b81
37e8af7
3b55ec9
74ecdbf
e002e38
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
When I suggested making this immutable, I was wrong.
Looking at how much more complex the code had to become, that was a mistake on my part.
you can probably just revert that commit
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.
Probably better not to do that?
push!
will resize for you.And it has heuristics to better handle the resizing
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.
Not sure I understand what you mean, do you mean to move
assure!
intopush!
I guess that's ok since I don't think it's used anywhere elseThere 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.
no, I mean why are we doing
resize!
thenpush!
?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.
Before this was to generate the
undef
s but now indeed i'm just usingpush!
aftersizehint!
, I don't know if that's optimalThere 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.
it is, basically,
Marginally better is
resize!
+ assignment to indexbut you missout on the heuristics of how
push!
will do extra resizing under the hood when it has to grow (I thinksizehint!
actaully might also block those since it iwill grow early)and that extra reizinging under the hood gives speedup between seperate calls
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.
did a quick benchmark with doing
resize!
followed by assignment to index, difference is basically swallowed by noise on the benchmark it seems.