-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
lib: add internalUtil.kEmptyObject
#43159
Conversation
Review requested:
|
What if our own code later assigns to a defaulted-to nullObject? It won't throw and the assignment will be lost. Do we enforce not modifying parameters through lint rules? Should we when nullObject was used? |
I believe assignment to a frozen object throws in strict mode? Are there places where it doesn't? Of course this object is not supposed to be exposed to usedland in any way, nor used in any scenario where there is a slightest chance to mutate it. |
I only tried with likewise constructed null object in REPL. |
It throws for me in REPL as well: Welcome to Node.js v18.1.0.
Type ".help" for more information.
> Object.assign(Object.freeze(Object.create(null)), { blep: 'blop' });
Uncaught TypeError: Cannot add property blep, object is not extensible
at Function.assign (<anonymous>) |
Maybe it should be named |
internalUtil.nullObject
internalUtil.kEmptyObject
Sure, renamed. I'm still tracing some tangled places in |
Trying a simpler approach, one that could be more likely used. > o=Object.freeze(Object.create(null))
[Object: null prototype] {}
> o.foo = 'bar'
'bar'
> o.foo
undefined |
Right, it doesn't run in strict mode by default. 😅 $ node --use_strict
Welcome to Node.js v18.1.0.
Type ".help" for more information.
> const o = Object.freeze(Object.create(null));
undefined
> o.blep = 'blop';
Uncaught TypeError: Cannot add property blep, object is not extensible Both approaches are covered by test: node/test/parallel/test-internal-util-objects.js Lines 76 to 87 in 5e914f2
|
Thank you for your diligence. I've learned something ;) |
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 highly suspect this will cause major performance regressions as frozen objects are slower than their traditional counterparts.
This should be deeply investigated for regressions before landing.
AFAIK performance hit from freezing was a thing in the past, but in modern v8 there's no difference anymore. Synthetic testing on 18.1.0 and master shows [fastest] To be safe, it would make sense to not backport this.
Aside from |
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1142/ EDIT: there are a few benchmark impacted by this change, overall it's pretty mild though:
Full resullts
|
Amazing progress, I might have missed it. Have you got a link? |
Not 100% sure if these are exhaustive or there were follow-ups: (for my tests I ran destructuring, not iterating) Edit: according to https://bugs.chromium.org/p/chromium/issues/detail?id=980227, there still are issues with some particular operations. I tried to benchmark their 1000-element array in Chromium and got this (higher is better):
But these operations shouldn't be applicable to this partucular empty object. |
5fea108
to
ae13f10
Compare
This comment was marked as outdated.
This comment was marked as outdated.
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2939/ Just in case of some sort of really elusive errors. |
PR-URL: #43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: #43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
PR-URL: nodejs/node#43159 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]>
Problem
There are lots of places in core where we have to pass an optional "pre-ES6 map"-style object with optional properties.
To avoid verbose boilerplate code or overusing of optional chaining, and to allow destructuring, we need a fallback value of
{}
, thus creating a throwaway object:It is also highly affected by prototype tampering, e.g.
So we have to prefer
Object.create(null)
, which is slower and still creates a throwaway object on every usage.Solution
A single immutable object that guarantees any property to be
undefined
.