Cross realm symbols #1243
Merged
Cross realm symbols #1243
Conversation
This file contains 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
HalidOdat
added
enhancement
HalidOdat
requested review from
Razican,
jasonwilliams,
Lan2u,
RageKnify,
0x7D2B and
tofpie
Test262 conformance changes:
|
Benchmark for e4b198bClick to view benchmark
|
Razican
requested changes
May 10, 2021
There was a problem hiding this comment.
Looking very good @HalidOdat. This will be very helpful when implementing the rest of the test262 suite with the shared object that should be able to create new realms, for example.
I only added a couple of comments on documentation, and opened a question on whether we should be using the Default trait for an empty new() function, mostly related to this clippy lint.
Benchmark for b9c478eClick to view benchmark
|
Razican
added a commit
that referenced
this pull request
May 22, 2021
* Cross context symbols * Remove redundant '_symbol' suffix for well known symbol getters * Fix symbol equality and hashing * Add documentation
Razican
added a commit
that referenced
this pull request
May 22, 2021
* Cross context symbols * Remove redundant '_symbol' suffix for well known symbol getters * Fix symbol equality and hashing * Add documentation
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.
Well known Symbols are able to be used between contexts (in the same thread) interchangeably, as described in the spec (
"Unless otherwise specified, well-known symbols values are shared by all realms"). The spec only says this about well known symbols, but it's better to have all symbols be interchangeable rather than a few, since we will run into some hash collisions for later created symbols.It changes the following:
_symbolsuffix for well known symbol getters.PartialEqforSymbolso it only checks equality for hashes (which are unique).HashforSymbolso it only hashes theSymbol's hash.The two fixes (
PartialEqandHash) might gives us a performance boots since we don't need to compare and hash the symbol description. (now we only compare and hash a u64, instead of the string description too).