-
Notifications
You must be signed in to change notification settings - Fork 13.2k
Clean up ObjectFlags enum #43732
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
Clean up ObjectFlags enum #43732
Conversation
|
Does this break the API? Previously internal |
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 96ad79c. You can monitor the build here. Update: The results are in! |
Yes, in theory it is a breaking change because a couple of visible flags now require a check of |
|
@sandersn Here they are:Comparison Report - master..43732
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 honestly surprised there wasn't anywhere we had to introduce a TypeFlags check.
|
I was curious so I ran the perf tests; this doesn't make a performance difference for x64, at least on any of the versions of node we test on. |
|
@sandersn Yeah, the offending flag is |
This PR cleans up the
ObjectFlagsenum. Specifically, it organizes the flags such that bit positions are reused to have different meanings in different types. This frees up several bit positions and ensures that no flags use bit positions 30 and above. We previously had a flag with the value1 << 30, the use of which adversely impacted performance because the VM boxes values in that range.