Skip to content

util: avoid out-of-bounds arguments index access #10569

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 5, 2017
Merged

util: avoid out-of-bounds arguments index access #10569

merged 1 commit into from
Jan 5, 2017

Conversation

@not-an-aardvark
Copy link
Contributor

@not-an-aardvark not-an-aardvark commented Jan 2, 2017 •
edited
Loading

This updates util.inspect() to avoid accessing out-of-range indices of the arguments object, which is known to cause optimization bailout.

Based on an average of 10 runs of the benchmark in benchmark/util/inspect.js, this change improves the performance of util.inspect by about 10%.

Relates to #10323

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

util
@nodejs-github-bot nodejs-github-bot added util Issues and PRs related to the built-in util module. lts-watch-v6.x labels Jan 2, 2017
@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Jan 2, 2017 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/5656/
@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Jan 2, 2017

Benchmark results:

Without this change:

$ for ((i=0;i<10;i+=1)); do ./node benchmark/util/inspect.js; done util/inspect.js n=5000000: 59,923 util/inspect.js n=5000000: 68,489 util/inspect.js n=5000000: 66,138 util/inspect.js n=5000000: 68,259 util/inspect.js n=5000000: 66,395 util/inspect.js n=5000000: 64,827 util/inspect.js n=5000000: 69,548 util/inspect.js n=5000000: 73,541 util/inspect.js n=5000000: 66,892 util/inspect.js n=5000000: 65,717

With this change:

$ for ((i=0;i<10;i+=1)); do ./node benchmark/util/inspect.js; done util/inspect.js n=5000000: 73,957 util/inspect.js n=5000000: 74,728 util/inspect.js n=5000000: 73,496 util/inspect.js n=5000000: 73,604 util/inspect.js n=5000000: 74,919 util/inspect.js n=5000000: 66,369 util/inspect.js n=5000000: 78,040 util/inspect.js n=5000000: 70,583 util/inspect.js n=5000000: 72,164 util/inspect.js n=5000000: 71,974
@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Jan 2, 2017

Accidentally pushed an older commit that didn't contain local changes, so CI failed. Re-pushed, new CI: https://ci.nodejs.org/job/node-test-pull-request/5657/
@mscdex
Copy link
Contributor

mscdex commented Jan 2, 2017

LGTM
1 similar comment
@JacksonTian
Copy link
Contributor

JacksonTian commented Jan 2, 2017

LGTM
@mscdex mscdex added the performance Issues and PRs related to the performance of Node.js. label Jan 3, 2017
@jasnell
Copy link
Member

jasnell commented Jan 4, 2017

Previous CI run had some red... running again just to be safe: https://ci.nodejs.org/job/node-test-pull-request/5701/
@not-an-aardvark
util: avoid out-of-bounds arguments index access
26f2a6e 
This updates util.inspect() to avoid accessing out-of-range indices of the `arguments` object, which is known to cause optimization bailout. Based on an average of 10 runs of the benchmark in `benchmark/util/inspect.js`, this change improves the performance of `util.inspect` by about 10%. Relates to nodejs#10323 PR-URL: nodejs#10569 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jackson Tian <shyvo1987@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@not-an-aardvark not-an-aardvark merged commit 26f2a6e into nodejs:master Jan 5, 2017
@not-an-aardvark
Copy link
Contributor Author

not-an-aardvark commented Jan 5, 2017

Landed in 26f2a6e
@not-an-aardvark not-an-aardvark deleted the util-inspect-arguments branch January 5, 2017 06:54
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@not-an-aardvark
util: avoid out-of-bounds arguments index access
46ca669 
This updates util.inspect() to avoid accessing out-of-range indices of the `arguments` object, which is known to cause optimization bailout. Based on an average of 10 runs of the benchmark in `benchmark/util/inspect.js`, this change improves the performance of `util.inspect` by about 10%. Relates to nodejs#10323 PR-URL: nodejs#10569 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jackson Tian <shyvo1987@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 19, 2017
@not-an-aardvark
util: avoid out-of-bounds arguments index access
74740b2 
This updates util.inspect() to avoid accessing out-of-range indices of the `arguments` object, which is known to cause optimization bailout. Based on an average of 10 runs of the benchmark in `benchmark/util/inspect.js`, this change improves the performance of `util.inspect` by about 10%. Relates to nodejs#10323 PR-URL: nodejs#10569 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jackson Tian <shyvo1987@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 24, 2017
@not-an-aardvark
util: avoid out-of-bounds arguments index access
4e52ebd 
This updates util.inspect() to avoid accessing out-of-range indices of the `arguments` object, which is known to cause optimization bailout. Based on an average of 10 runs of the benchmark in `benchmark/util/inspect.js`, this change improves the performance of `util.inspect` by about 10%. Relates to nodejs#10323 PR-URL: nodejs#10569 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jackson Tian <shyvo1987@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@not-an-aardvark
util: avoid out-of-bounds arguments index access
d628f3a 
This updates util.inspect() to avoid accessing out-of-range indices of the `arguments` object, which is known to cause optimization bailout. Based on an average of 10 runs of the benchmark in `benchmark/util/inspect.js`, this change improves the performance of `util.inspect` by about 10%. Relates to nodejs#10323 PR-URL: nodejs#10569 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Jackson Tian <shyvo1987@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 29, 2017
Merged
@vsemozhetbyt vsemozhetbyt mentioned this pull request Feb 2, 2017
Closed
@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 7, 2017

should this be backported?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance Issues and PRs related to the performance of Node.js. util Issues and PRs related to the built-in util module.

9 participants

@not-an-aardvark @mscdex @JacksonTian @jasnell @MylesBorins @evanlucas @Fishrock123 @cjihrig @nodejs-github-bot