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: avoid out-of-bounds arguments index access #10569

Merged

Conversation

not-an-aardvark
Copy link
Contributor

@not-an-aardvark not-an-aardvark commented Jan 2, 2017

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

@not-an-aardvark
Copy link
Contributor Author

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

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

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/

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
Copy link
Contributor Author

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
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
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
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 27, 2017
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
@MylesBorins
Copy link
Contributor

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants