-
-
Notifications
You must be signed in to change notification settings - Fork 31.6k
lib: make new SafeSet(arr)
safe
#57895
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
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
d57bd99
to
fa04710
Compare
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.
LGTM!
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.
lgtm
(i assume the coverage failures mean i need to add some tests; if nobody has any objections to the PR then i'll ofc add those before attempting to land) |
It would be worth specifying in node/doc/contributing/primordials.md Lines 429 to 430 in c42d846
and probably add it to the list of culprit for performance issues (I guess we should first established whether it's indeed the case) node/doc/contributing/primordials.md Lines 108 to 113 in c42d846
Also, build is failing. |
Regarding the primordials.md comment, those are about the builtins, not the SafeX constructors, so I don't think it makes sense there - and I agree that we should wait until performance issues are identified before noting a performance culprit. I believe I've addressed the rest of the concerns (except probably the failing build, which I'll iterate on). I haven't applied this to SafeWeakSet, because there's only 2 instances of it in the codebase and neither are adding at construction time - I'm happy to add it in the future if it proves useful. |
This section is using the |
ah, true. should i change the example to something else, and then add a note indicating that SafeSet is immune to this? |
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.
oow, nice ergonomic improvement indeed 🙌
constructor(i) { super(i); } // eslint-disable-line no-useless-constructor | ||
constructor(i) { | ||
super( | ||
i && ArrayIsArray(i) ? |
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.
nit: the truthy test is probably superfluous. I can't find the implementation of ArrayIsArray
, but it probably already does that, so this micro-op is likely actually a micro-deop.
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.
my assumption is that a truthiness check is much cheaper than any function call, but if a benchmark indicates that's untrue i'd be happy to change it in the future.
This has the direct ergonomic benefit of being able to safely pass an array to
new SafeSet()
.It has the unintended but desirable benefit of making a lot of existing post-startup calls to
new SafeSet
with an array more robust.This helps with #57876, and whichever lands first, the other will want to rebase on top of it.