-
Notifications
You must be signed in to change notification settings - Fork 29.2k
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
http: remove prototype primordials #53698
Conversation
Review requested:
|
e18e36a
to
2b386b3
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.
❤️
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
Is there a benchmark that would measure the difference that this change makes? |
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1576/ Results
|
So if I’m reading this right, most benchmarks are slightly faster and a few are dramatically faster, almost twice as fast? If that’s the case then we should remove primordials all across the codebase, especially in hot paths. I think the presumption should be that there needs to be a strong argument for the few places where we wouldn’t remove them. |
Co-authored-by: Yagiz Nizipli <[email protected]>
e1cc780
to
3c280e0
Compare
Commit Queue failed- Loading data for nodejs/node/pull/53698 ✔ Done loading data for nodejs/node/pull/53698 ----------------------------------- PR info ------------------------------------ Title http: remove prototype primordials (#53698) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch aduh95:http-bye-bye-primordials -> nodejs:main Labels lib / src, author ready, needs-ci, commit-queue-squash, dont-land-on-v18.x Commits 3 - http: remove prototype primordials - document areas of the codebase with prototype pollution - update linter config Committers 1 - Antoine du Hamel PR-URL: https://github.com/nodejs/node/pull/53698 Reviewed-By: Yagiz Nizipli Reviewed-By: Robert Nagy Reviewed-By: Moshe Atlow Reviewed-By: Marco Ippolito Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell Reviewed-By: Rafael Gonzaga ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/53698 Reviewed-By: Yagiz Nizipli Reviewed-By: Robert Nagy Reviewed-By: Moshe Atlow Reviewed-By: Marco Ippolito Reviewed-By: Luigi Pinca Reviewed-By: Matteo Collina Reviewed-By: Benjamin Gruenbaum Reviewed-By: James M Snell Reviewed-By: Rafael Gonzaga -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - http: remove prototype primordials ⚠ - document areas of the codebase with prototype pollution ⚠ - update linter config ℹ This PR was created on Tue, 02 Jul 2024 17:24:00 GMT ✔ Approvals: 9 ✔ - Yagiz Nizipli (@anonrig): https://github.com/nodejs/node/pull/53698#pullrequestreview-2154462664 ✔ - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/53698#pullrequestreview-2154500028 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/53698#pullrequestreview-2154501177 ✔ - Marco Ippolito (@marco-ippolito) (TSC): https://github.com/nodejs/node/pull/53698#pullrequestreview-2154503031 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/53698#pullrequestreview-2154520650 ✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/53698#pullrequestreview-2154573446 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53698#pullrequestreview-2154632284 ✔ - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53698#pullrequestreview-2157912950 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/53698#pullrequestreview-2159299739 ✔ Last GitHub CI successful ℹ Last Benchmark CI on 2024-07-03T21:54:18Z: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1576/ ℹ Last Full PR CI on 2024-07-05T18:46:03Z: https://ci.nodejs.org/job/node-test-pull-request/60102/ - Querying data for job/node-test-pull-request/60102/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/9817906165 |
Landed in cbd2c38 |
Co-authored-by: Yagiz Nizipli <[email protected]> PR-URL: #53698 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Co-authored-by: Yagiz Nizipli <[email protected]> PR-URL: #53698 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Co-authored-by: Yagiz Nizipli <[email protected]> PR-URL: #53698 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Yagiz and I paired to remove primordials in http files, as those are really just a JS lib that happens to be in core, not something for which prototype pollution is a big concern.