GeezerVison is showing github.com — util: prefer `Reflect.ownKeys(…)` in `util.inspect(…)` by ExE-Boss · Pull Request #36740 · nodejs/node
Conversation
ExE-Boss
commented
Jan 2, 2021
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
Using Reflect.ownKeys(obj) instead of:
const keys = Object.getOwnPropertyNames(obj);
const symbols = Object.getOwnPropertySymbols(obj);
if (symbols.length !== 0) {
keys.push(...symbols);
}is more efficient, as it only has to call obj.[[OwnPropertyKeys]]() once and return the resulting List as an Array, instead of calling it twice and returning two separate arrays, which have the string and symbol keys filtered: https://tc39.es/ecma262/#sec-getownpropertykeys, which are then joined back using %Array.prototype.push%.
Checklist
-
make -j4 test (UNIX), or vcbuild test (Windows) passes
- commit message follows commit guidelines
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
bricss reacted with thumbs up emoji
ExE-Boss
changed the title
util: prefer Reflect.ownKeys(…)
util: prefer Reflect.ownKeys(…) in util.inspect(…)
Jan 2, 2021
mscdex
commented
Jan 2, 2021
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
This seems to introduce a performance regression:
util/inspect.js option='showHidden' method='Object_deep_ln' n=20000 *** -11.38 % ±2.30% ±3.06% ±3.99%
|
aduh95 and ExE-Boss reacted with eyes emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
@ExE-Boss Reflect is not as optimized in V8, if I remember correct. I tried to use ownKeys and used the current implementation due to the performance impact.
|
himself65 reacted with thumbs up emoji
ExE-Boss reacted with eyes emoji
All reactions
-
👍
1 reaction
-
👀
1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
The commit responsible for that is v8/v8@f9868ea.
Oddly enough, Object.getOwnPropertySymbols still uses the slow path.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
aduh95
commented
Jan 19, 2021
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Bnechmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/906/
EDIT: no significant regressions anymore 🎉
Benchmark results
confidence improvement accuracy (*) (**) (***)
util/inspect-array.js type='denseArray' len=100000 n=500 2.82 % ±4.01% ±5.35% ±6.98%
util/inspect-array.js type='denseArray' len=100 n=500 -2.63 % ±5.69% ±7.57% ±9.85%
util/inspect-array.js type='denseArray_showHidden' len=100000 n=500 -2.87 % ±4.58% ±6.09% ±7.92%
util/inspect-array.js type='denseArray_showHidden' len=100 n=500 0.35 % ±4.47% ±5.95% ±7.75%
util/inspect-array.js type='mixedArray' len=100000 n=500 -0.26 % ±2.32% ±3.09% ±4.02%
util/inspect-array.js type='mixedArray' len=100 n=500 0.54 % ±5.15% ±6.86% ±8.94%
util/inspect-array.js type='sparseArray' len=100000 n=500 1.83 % ±3.60% ±4.80% ±6.26%
util/inspect-array.js type='sparseArray' len=100 n=500 0.72 % ±6.69% ±8.92% ±11.65%
util/inspect.js option='colors' method='Array' n=20000 0.53 % ±2.53% ±3.38% ±4.44%
util/inspect.js option='colors' method='Date' n=20000 0.98 % ±4.81% ±6.41% ±8.35%
util/inspect.js option='colors' method='Error' n=20000 0.77 % ±4.07% ±5.42% ±7.05%
util/inspect.js option='colors' method='Number' n=20000 1.82 % ±2.80% ±3.72% ±4.85%
util/inspect.js option='colors' method='Object_deep_ln' n=20000 0.03 % ±1.71% ±2.27% ±2.95%
util/inspect.js option='colors' method='Object_empty' n=20000 2.75 % ±5.69% ±7.58% ±9.89%
util/inspect.js option='colors' method='Object' n=20000 0.08 % ±2.58% ±3.43% ±4.46%
util/inspect.js option='colors' method='Set' n=20000 -0.50 % ±4.49% ±5.99% ±7.80%
util/inspect.js option='colors' method='String_boxed' n=20000 -0.04 % ±3.49% ±4.65% ±6.06%
util/inspect.js option='colors' method='String_complex' n=20000 1.16 % ±3.43% ±4.58% ±5.98%
util/inspect.js option='colors' method='String' n=20000 3.03 % ±3.92% ±5.24% ±6.88%
util/inspect.js option='colors' method='TypedArray_extra' n=20000 -1.70 % ±1.86% ±2.47% ±3.22%
util/inspect.js option='colors' method='TypedArray' n=20000 0.71 % ±1.54% ±2.05% ±2.67%
util/inspect.js option='none' method='Array' n=20000 2.11 % ±7.43% ±9.91% ±12.93%
util/inspect.js option='none' method='Date' n=20000 0.15 % ±4.87% ±6.48% ±8.43%
util/inspect.js option='none' method='Error' n=20000 0.83 % ±4.79% ±6.37% ±8.29%
util/inspect.js option='none' method='Number' n=20000 1.33 % ±3.58% ±4.76% ±6.20%
util/inspect.js option='none' method='Object_deep_ln' n=20000 -0.99 % ±1.95% ±2.59% ±3.37%
util/inspect.js option='none' method='Object_empty' n=20000 1.89 % ±5.14% ±6.86% ±8.97%
util/inspect.js option='none' method='Object' n=20000 0.39 % ±3.95% ±5.26% ±6.86%
util/inspect.js option='none' method='Set' n=20000 3.23 % ±5.14% ±6.86% ±8.95%
util/inspect.js option='none' method='String_boxed' n=20000 -0.31 % ±4.23% ±5.63% ±7.33%
util/inspect.js option='none' method='String_complex' n=20000 -2.17 % ±3.98% ±5.30% ±6.90%
util/inspect.js option='none' method='String' n=20000 -0.73 % ±4.14% ±5.52% ±7.19%
util/inspect.js option='none' method='TypedArray_extra' n=20000 0.54 % ±5.03% ±6.70% ±8.73%
util/inspect.js option='none' method='TypedArray' n=20000 2.79 % ±6.36% ±8.55% ±11.31%
util/inspect.js option='showHidden' method='Array' n=20000 -0.34 % ±1.52% ±2.04% ±2.68%
util/inspect.js option='showHidden' method='Date' n=20000 1.80 % ±4.49% ±5.98% ±7.79%
util/inspect.js option='showHidden' method='Error' n=20000 -2.35 % ±3.57% ±4.77% ±6.24%
util/inspect.js option='showHidden' method='Number' n=20000 -2.05 % ±2.76% ±3.67% ±4.77%
util/inspect.js option='showHidden' method='Object_deep_ln' n=20000 -0.64 % ±2.67% ±3.56% ±4.63%
util/inspect.js option='showHidden' method='Object_empty' n=20000 * 5.26 % ±4.36% ±5.80% ±7.56%
util/inspect.js option='showHidden' method='Object' n=20000 2.68 % ±3.76% ±5.01% ±6.53%
util/inspect.js option='showHidden' method='Set' n=20000 4.12 % ±5.72% ±7.62% ±9.93%
util/inspect.js option='showHidden' method='String_boxed' n=20000 * -4.48 % ±4.47% ±5.94% ±7.74%
util/inspect.js option='showHidden' method='String_complex' n=20000 * 3.95 % ±3.53% ±4.69% ±6.11%
util/inspect.js option='showHidden' method='String' n=20000 -1.28 % ±5.35% ±7.12% ±9.28%
util/inspect.js option='showHidden' method='TypedArray_extra' n=20000 -1.53 % ±2.13% ±2.84% ±3.70%
util/inspect.js option='showHidden' method='TypedArray' n=20000 0.44 % ±1.79% ±2.39% ±3.11%
util/inspect-proxy.js isProxy=0 showProxy=0 n=100000 -1.17 % ±4.08% ±5.43% ±7.07%
util/inspect-proxy.js isProxy=0 showProxy=1 n=100000 ** 2.33 % ±1.73% ±2.30% ±2.99%
util/inspect-proxy.js isProxy=1 showProxy=0 n=100000 -1.05 % ±2.20% ±2.93% ±3.82%
util/inspect-proxy.js isProxy=1 showProxy=1 n=100000 0.94 % ±3.32% ±4.44% ±5.83%
Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 51 comparisons, you can thus
expect the following amount of false-positive results:
2.55 false positives, when considering a 5% risk acceptance (*, **, ***),
0.51 false positives, when considering a 1% risk acceptance (**, ***),
0.05 false positives, when considering a 0.1% risk acceptance (***)
|
ExE-Boss reacted with hooray emoji
BridgeAR reacted with confused emoji
All reactions
-
🎉
1 reaction
-
😕
1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
ExE-Boss reacted with hooray emoji
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
EDIT: no significant regressions anymore 🎉
The benchmarks never hit the changed code path.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
aduh95
aduh95 approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Labels
author ready
PRs with CI started, the required approvals, and no outstanding review comments.
util
Issues and PRs related to the built-in util module.
Milestone
No milestone
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
|