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
Performance improvements for IdDict #51091
base: master
Are you sure you want to change the base?
Performance improvements for IdDict #51091
Changes from 13 commits
717388b
27dcc24
a31404b
d5fba6f
d17b27b
bb360d6
f377b7e
e34913a
46daa2d
cd5f495
ad5a02a
c3047c1
3a428ad
25e7410
6cbd3f0
e864462
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.
why does this need to be implemented in C?
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.
I'm not sure how to call
jl_table_assign_bp
directly from Julia as it can changed.ht
.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.
There used to be ways, but not anymore with the generational GC. The usual way now to implement this is to switch the return object around as such:
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.
Can you change this around still also? It will avoid us from needing to reflect the iddict struct into C
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.
Just to note, I believe this would remove the ~20% speedup (see comments above) we get from not having to heap allocate for
setindex!
. Happy to make this change if that's fine.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.
There seems to be a discrepancy here in the state of the IdDict during this convert call or if this convert failed. The
ht_keyindex!
call has fully inserted the key, but not the value. ForDict
, neither has occurred yet, and all insertion and accounting happens at once in_setindex!
. But in IdDict here, it looks like half of the insertion occurs inht_keyindex!
(effectively inserting it in the deleted state) but then the final accounting update for it doesn't happen until after the_setindex!
call later in this method. Do we updatendel
over this call so the value is consistent?There also may need to be an
age
counter added to the IdDict, so it can detect concurrent modifications that happen in thisconvert
call (or move this convert call back to the top of the function)