Skip to content
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

util: prefer Reflect.ownKeys(…) in util.inspect(…) #36740

Merged

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jan 2, 2021

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

@nodejs-github-bot nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jan 2, 2021
@ExE-Boss ExE-Boss changed the title util: prefer Reflect.ownKeys(…) util: prefer Reflect.ownKeys(…) in util.inspect(…) Jan 2, 2021
aduh95
aduh95 previously approved these changes Jan 2, 2021
@mscdex
Copy link
Contributor

mscdex commented Jan 2, 2021

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%

lib/internal/util/inspect.js Outdated Show resolved Hide resolved
@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2021

@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.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jan 3, 2021

The commit responsible for that is v8/v8@f9868ea.


Oddly enough, Object.getOwnPropertySymbols still uses the slow path.

@aduh95 aduh95 dismissed their stale review January 10, 2021 23:06

Dismissing due to performance concerns.

@ExE-Boss ExE-Boss force-pushed the lib/util/inspect/prefer-reflect-ownkeys branch from e1cce62 to 6c88555 Compare January 19, 2021 02:21
@aduh95
Copy link
Contributor

aduh95 commented Jan 19, 2021

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

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 19, 2021
@nodejs-github-bot
Copy link
Collaborator

PR-URL: nodejs#36740
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@aduh95 aduh95 force-pushed the lib/util/inspect/prefer-reflect-ownkeys branch from 6c88555 to e9944e9 Compare January 20, 2021 00:25
@aduh95
Copy link
Contributor

aduh95 commented Jan 20, 2021

Landed in e9944e9

@aduh95 aduh95 closed this Jan 20, 2021
@aduh95 aduh95 merged commit e9944e9 into nodejs:master Jan 20, 2021
@ExE-Boss ExE-Boss deleted the lib/util/inspect/prefer-reflect-ownkeys branch January 20, 2021 00:30
@BridgeAR
Copy link
Member

EDIT: no significant regressions anymore 🎉

The benchmarks never hit the changed code path.

ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
PR-URL: #36740
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
ruyadorno pushed a commit that referenced this pull request Jan 25, 2021
PR-URL: #36740
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants