FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

util: avoid out-of-bounds arguments index access by not-an-aardvark · Pull Request #10569 · nodejs/node · GitHub

/ node Public

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

Merged
not-an-aardvark merged 1 commit into
nodejs:masterfrom
not-an-aardvark:util-inspect-arguments
Jan 5, 2017
ConversationCommits (1)ChecksFiles changed
Merged

util: avoid out-of-bounds arguments index access#10569
not-an-aardvark merged 1 commit into
nodejs:masterfrom
not-an-aardvark:util-inspect-arguments

Conversation

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

Copy link
Copy Markdown
Contributor

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 added util Issues and PRs related to the built-in util module. lts-watch-v6.x labels Jan 2, 2017

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

Copy link
Copy Markdown
Contributor Author

CI: https://ci.nodejs.org/job/node-test-pull-request/5656/

not-an-aardvark commented Jan 2, 2017

Copy link
Copy Markdown
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 commented Jan 2, 2017

Copy link
Copy Markdown
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 commented Jan 2, 2017

Copy link
Copy Markdown
Contributor

LGTM

1 similar comment

JacksonTian commented Jan 2, 2017

Copy link
Copy Markdown
Contributor

LGTM

mscdex added the performance Issues and PRs related to the performance of Node.js. label Jan 3, 2017

jasnell commented Jan 4, 2017

Copy link
Copy Markdown
Member

Previous CI run had some red... running again just to be safe: https://ci.nodejs.org/job/node-test-pull-request/5701/

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 merged commit 26f2a6e into nodejs:master Jan 5, 2017

not-an-aardvark commented Jan 5, 2017

Copy link
Copy Markdown
Contributor Author

Landed in 26f2a6e

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
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
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
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 mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
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 mentioned this pull request Jan 29, 2017
Merged
vsemozhetbyt mentioned this pull request Feb 2, 2017
Closed

MylesBorins commented Mar 7, 2017

Copy link
Copy Markdown
Contributor

should this be backported?

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
evanlucas evanlucas approved these changes
Fishrock123 Fishrock123 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

Back | FazBrowse Home | New Git URL