FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

lib: make `new SafeSet(arr)` safe by ljharb · Pull Request #57895 · nodejs/node · GitHub

/ node Public

lib: make new SafeSet(arr) safe - #57895

Draft
ljharb wants to merge 1 commit into
nodejs:mainfrom
ljharb:safeset
ConversationCommits (1)ChecksFiles changed
Draft

lib: make new SafeSet(arr) safe#57895
ljharb wants to merge 1 commit into
nodejs:mainfrom
ljharb:safeset

Conversation

ljharb commented Apr 16, 2025

Copy link
Copy Markdown
Member

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.

ljharb added the lib / src Issues and PRs related to general changes in the lib or src directory. label Apr 16, 2025
ljharb requested review from BridgeAR and aduh95 April 16, 2025 05:58

nodejs-github-bot commented Apr 16, 2025

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/test_runner
  • @nodejs/tsc

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 16, 2025
ljharb force-pushed the safeset branch 2 times, most recently from d57bd99 to fa04710 Compare April 16, 2025 06:06
ShogunPanda approved these changes Apr 16, 2025
View reviewed changes

ShogunPanda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM!

mcollina approved these changes Apr 16, 2025
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

ljharb commented Apr 16, 2025

Copy link
Copy Markdown
Member Author

(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)

aduh95 reviewed Apr 16, 2025
View reviewed changes
Comment thread lib/internal/error_serdes.js Outdated
aduh95 reviewed Apr 16, 2025
View reviewed changes
Comment thread doc/contributing/primordials.md Outdated
aduh95 added the needs-benchmark-ci PR that need a benchmark CI run. label Apr 16, 2025

aduh95 commented Apr 16, 2025

Copy link
Copy Markdown
Contributor

It would be worth specifying in primordials.md that SafeSet constructor is an odd one out

node/doc/contributing/primordials.md

Lines 429 to 430 in c42d846

<summary><code>%Map%</code>, <code>%Set%</code>, <code>%WeakMap%</code>, and
<code>%WeakSet%</code> constructors iterate over an array</summary>

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

## Primordials with known performance issues
One of the reasons why the current Node.js API is not completely tamper-proof is
performance: sometimes the use of primordials can cause performance regressions
with V8, which when in a hot code path, could significantly decrease the
performance of code in Node.js.

Also, build is failing.

ExE-Boss reviewed Apr 16, 2025
View reviewed changes
Comment thread lib/internal/per_context/primordials.js Outdated
Comment thread lib/internal/per_context/primordials.js

ljharb commented Apr 16, 2025

Copy link
Copy Markdown
Member Author

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.

aduh95 commented Apr 16, 2025

Copy link
Copy Markdown
Contributor

Regarding the primordials.md comment, those are about the builtins, not the SafeX constructors, so I don't think it makes sense there

This section is using the SafeSet constructor as an example though

ljharb commented Apr 16, 2025

Copy link
Copy Markdown
Member Author

ah, true. should i change the example to something else, and then add a note indicating that SafeSet is immune to this?

JakobJingleheimer approved these changes Apr 16, 2025
View reviewed changes

JakobJingleheimer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

oow, nice ergonomic improvement indeed 🙌

Comment thread lib/internal/per_context/primordials.js
constructor(i) { super(i); } // eslint-disable-line no-useless-constructor
constructor(i) {
super(
i && ArrayIsArray(i) ?

JakobJingleheimer Apr 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ljharb Apr 16, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ljharb marked this pull request as draft April 18, 2025 19:21

github-actions Bot commented Apr 20, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

github-actions Bot added the stale label Apr 20, 2026
github-actions Bot removed the stale label Apr 21, 2026

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

github-actions Bot added the stale label Jul 28, 2026
avivkeller closed this Aug 9, 2026
ljharb reopened this Aug 17, 2026
ljharb removed the stale label Aug 17, 2026
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

aduh95 aduh95 left review comments
mcollina mcollina approved these changes
ShogunPanda ShogunPanda approved these changes
Qard Qard approved these changes
jasnell jasnell approved these changes
JakobJingleheimer JakobJingleheimer approved these changes
atlowChemi atlowChemi approved these changes
BridgeAR Awaiting requested review from BridgeAR
+2 more reviewers
ExE-Boss ExE-Boss left review comments
jsumners-nr jsumners-nr approved these changes
Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-benchmark-ci PR that need a benchmark CI run. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Back | FazBrowse Home | New Git URL