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
Devirtualize TokenMap #47274
Devirtualize TokenMap #47274
Changes from 1 commit
b3afe69
7a9aee7
86dcb4b
7262313
658fbe3
7753168
0d53ed8
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 is this safe? It seems very likely to me that _items could have been changed by this point.
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.
This is the only location
_items
is updated and it is done in a lock. TheVolatile.Write
is to ensure its not reordered with the write to_count
; so it's safe toVolatile.Read
them in reverse order inGetAllItems()
since_count
only ever increasesThere 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.
This doesn't seem particularly safe to me either.
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.
Consider that
AddItem
does volatile writes in the following order:_items
then_count
. This method now reads them volatiley in the other order_count
then_items
. That means by the time we read_count
here we know there are at least_count
elements in the_items
collection hence the array returned here will be well formed (nonull
values).There is a subtle invariant change from the previous version of the code:
AddItem
was executing then it would return the effect of completing theAddItem
methodAddItem
was executing then it may or may not return the effect of completing theAddItem
method.Don't believe we care about maintaining that invariant.
This file was deleted.
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 I understand why we would need both of these: it seems like just
IReferenceOrISignatureEquivalent
should be 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.
IReferenceOrISignature
usesReferenceEquals
for equals andIReferenceOrISignatureEquivalent
has a more complex equals, previously provided byMetadataEntityReferenceComparer.cs
The dictionary, hashtable and concurrentdictionary will devirtualize (and potentially inline) the equals for a struct when the Comparer is unspecified (if it also implements
IEquatable
) in .NET Core; whereas specifying a Comparer will be a slower interface call.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.
This would be great info to have in a comment in the file itself :)
In reply to: 480276103 [](ancestors = 480276103)