-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: use null-prototype objects for property descriptors #43270
lib: use null-prototype objects for property descriptors #43270
Conversation
Review requested:
|
Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1149/ Results
|
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. did you also search for uses of ReflectDefineProperty
, or the second parameter to ObjectCreate
?
Good catch, I did not. |
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
This comment was marked as outdated.
This comment was marked as outdated.
// %Object.prototype%. | ||
ObjectSetPrototypeOf(propertiesValues[i], null); | ||
} | ||
sharedProperties[inspect].enumerable = false; |
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.
why mutate it instead of spreading it in like below?:
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.
Why not mutate it instead? If mutating is not an option, I think I'd prefer ObjectDefineProperty(TextDecoder.prototype, inspect, { __proto__: null, enumerable: false });
over your suggestion.
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.
mutation is something that both slows things down (by changing the object's shape) and is also conceptually distasteful, so in general it's nicer to create objects wholesale rather than ever altering one.
go with whatever you prefer, obviously
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.
Unless someone else wants to take a side, I think I'm going to keep it as is. I'm fine with whichever solution, but mutating sharedProperties
seems the most natural to me in this instance.
|
||
ObjectDefineProperties(TextDecoder.prototype, { | ||
decode: kEnumerableProperty, | ||
[inspect]: { __proto__: null, enumerable: false }, | ||
...sharedProperties, |
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.
...sharedProperties, | |
...sharedProperties, | |
[inspect]: { __proto__: null, ...sharedProperties[inspect], enumerable: false }, |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Landed in 06d8606 |
Refs: nodejs#42921 PR-URL: nodejs#43270 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
This comment was marked as spam.
This comment was marked as spam.
This would need a backport PR to land on v16.x-staging (assuming the conflicts don't depend on changes that should be backported first). |
* chore: bump node in DEPS to v16.17.0 * chore: fixup asar patch * lib: use null-prototype objects for property descriptors nodejs/node#43270 * src: make SecureContext fields private nodejs/node#43173 * crypto: remove Node.js-specific webcrypto extensions nodejs/node#43310 * test: refactor to top-level await nodejs/node#43500 * deps: cherry-pick two libuv fixes nodejs/node#43950 * src: slim down env-inl.h nodejs/node#43745 * util: add AggregateError.prototype.errors to inspect output nodejs/node#43646 * esm: improve performance & tidy tests nodejs/node#43784 * src: NodeArrayBufferAllocator delegates to v8's allocator nodejs/node#43594 * chore: update patch indices * chore: update filenames * src: refactor IsSupportedAuthenticatedMode nodejs/node#42368 * src: add --openssl-legacy-provider option nodejs/node#40478 * lib,src: add source map support for global eval nodejs/node#43428 * trace_events: trace net connect event nodejs/node#43903 * deps: update ICU to 71.1 nodejs/node#42655 This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093 * lib: give names to promisified exists() and question() nodejs/node#43218 * crypto: add CFRG curves to Web Crypto API nodejs/node#42507 * src: fix memory leak for v8.serialize nodejs/node#42695 This test does not work for Electron as they do not use V8's ArrayBufferAllocator. * buffer: fix atob input validation nodejs/node#42539 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Refs: nodejs/node#42921 PR-URL: nodejs/node#43270 Backport-PR-URL: nodejs/node#43804 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
* chore: bump node in DEPS to v16.17.0 * chore: fixup asar patch * lib: use null-prototype objects for property descriptors nodejs/node#43270 * src: make SecureContext fields private nodejs/node#43173 * crypto: remove Node.js-specific webcrypto extensions nodejs/node#43310 * test: refactor to top-level await nodejs/node#43500 * deps: cherry-pick two libuv fixes nodejs/node#43950 * src: slim down env-inl.h nodejs/node#43745 * util: add AggregateError.prototype.errors to inspect output nodejs/node#43646 * esm: improve performance & tidy tests nodejs/node#43784 * src: NodeArrayBufferAllocator delegates to v8's allocator nodejs/node#43594 * chore: update patch indices * chore: update filenames * src: refactor IsSupportedAuthenticatedMode nodejs/node#42368 * src: add --openssl-legacy-provider option nodejs/node#40478 * lib,src: add source map support for global eval nodejs/node#43428 * trace_events: trace net connect event nodejs/node#43903 * deps: update ICU to 71.1 nodejs/node#42655 This fails the test because it's missing https://chromium-review.googlesource.com/c/chromium/deps/icu/+/3841093 * lib: give names to promisified exists() and question() nodejs/node#43218 * crypto: add CFRG curves to Web Crypto API nodejs/node#42507 * src: fix memory leak for v8.serialize nodejs/node#42695 This test does not work for Electron as they do not use V8's ArrayBufferAllocator. * buffer: fix atob input validation nodejs/node#42539 * src: fix ssize_t error from nghttp2.h nodejs/node#44393 Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Refs: nodejs/node#42921 PR-URL: nodejs/node#43270 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Refs: #42921