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

deps: backport 03ef3cd from V8 upstream #3165

Closed

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Oct 3, 2015

Reduces likelihood of the filename being truncated in /tmp/perf-*.map when
profiling with --perf_basic_prof.

R=@bnoordhuis

v8/v8@03ef3cd

Original commit message:
improve perf_basic_prof filename reporting

The buffer used for appending filenames to the string printed to the
perf_basic_prof log was unnecessarily too small. Bump it up to be at least
kUtf8BufferSize.

Truncation of filenames makes it really hard to work with profiles gathered on
Node.js. Because of the way Node.js works, you can have node module dependencies
in deeply nested directories. The last thing you want when investigating a
performance problem is to have script names be truncated.

This patch is a stop-gap. Ideally, I want no truncation of the filename at all
and use a dynamically growing buffer. That would be a larger change, and I
wanted to have a quick fix available that can be back-ported to Node.js LTS
release.

R=yangguo@chromium.org,yurys@chromium.org
BUG=

Review URL: https://codereview.chromium.org/1388543002

Cr-Commit-Position: refs/heads/master@{#31092}

Reduces likelihood of the filename being truncated in /tmp/perf-*.map when
profiling with --perf_basic_prof.

v8/v8@03ef3cd

Original commit message:
  improve perf_basic_prof filename reporting

  The buffer used for appending filenames to the string printed to the
  perf_basic_prof log was unnecessarily too small. Bump it up to be at least
  kUtf8BufferSize.

  Truncation of filenames makes it really hard to work with profiles gathered on
  Node.js. Because of the way Node.js works, you can have node module dependencies
  in deeply nested directories. The last thing you want when investigating a
  performance problem is to have script names be truncated.

  This patch is a stop-gap. Ideally, I want no truncation of the filename at all
  and use a dynamically growing buffer. That would be a larger change, and I
  wanted to have a quick fix available that can be back-ported to Node.js LTS
  release.

  R=yangguo@chromium.org,yurys@chromium.org
  BUG=

  Review URL: https://codereview.chromium.org/1388543002

  Cr-Commit-Position: refs/heads/master@{nodejs#31092}
@mscdex mscdex added the v8 engine Issues and PRs related to the V8 dependency. label Oct 3, 2015
@bnoordhuis
Copy link
Member

LGTM

ofrobots added a commit that referenced this pull request Oct 5, 2015
Reduces likelihood of the filename being truncated in /tmp/perf-*.map when
profiling with --perf_basic_prof.

PR-URL: #3165
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>

v8/v8@03ef3cd

Original commit message:
  improve perf_basic_prof filename reporting

  The buffer used for appending filenames to the string printed to the
  perf_basic_prof log was unnecessarily too small. Bump it up to be at least
  kUtf8BufferSize.

  Truncation of filenames makes it really hard to work with profiles gathered on
  Node.js. Because of the way Node.js works, you can have node module dependencies
  in deeply nested directories. The last thing you want when investigating a
  performance problem is to have script names be truncated.

  This patch is a stop-gap. Ideally, I want no truncation of the filename at all
  and use a dynamically growing buffer. That would be a larger change, and I
  wanted to have a quick fix available that can be back-ported to Node.js LTS
  release.

  R=yangguo@chromium.org,yurys@chromium.org
  BUG=

  Review URL: https://codereview.chromium.org/1388543002

  Cr-Commit-Position: refs/heads/master@{#31092}
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 5, 2015

Landed on master in 6fff47f.

@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 5, 2015

I forgot to run a CI, apologies. Here's the CI: https://ci.nodejs.org/job/node-test-pull-request/417/. As expected, the CI looks okay as we don't really run anything with --perf_basic_prof (to my knowledge).

@targos
Copy link
Member

targos commented Oct 6, 2015

Can this be closed?

@ofrobots ofrobots closed this Oct 6, 2015
@ofrobots
Copy link
Contributor Author

ofrobots commented Oct 7, 2015

Re-opening to revert this due to https://codereview.chromium.org/1390923004/

@ofrobots ofrobots deleted the fix-filename-truncation-perf branch October 14, 2015 18:35
ofrobots added a commit to ofrobots/node that referenced this pull request Oct 26, 2015
This is a reland of nodejs#3165. The patch abates
the truncation of script filenames in the perf-event output produced by V8.

V8 commits:
Original: v8/v8@03ef3cd
Reland: v8/v8@010897c

Original commit message:
  improve perf_basic_prof filename reporting

  The buffer used for appending filenames to the string printed to the
  perf_basic_prof log was unnecessarily too small. Bump it up to be at least
  kUtf8BufferSize.

  Truncation of filenames makes it really hard to work with profiles gathered on
  Node.js. Because of the way Node.js works, you can have node module dependencies
  in deeply nested directories. The last thing you want when investigating a
  performance problem is to have script names be truncated.

  This patch is a stop-gap. Ideally, I want no truncation of the filename at all
  and use a dynamically growing buffer. That would be a larger change, and I
  wanted to have a quick fix available that can be back-ported to Node.js LTS
  release.

  R=yangguo@chromium.org,yurys@chromium.org
  BUG=

  Review URL: https://codereview.chromium.org/1388543002
ofrobots added a commit that referenced this pull request Oct 28, 2015
This is a reland of #3165. The patch abates
the truncation of script filenames in the perf-event output produced by V8.

V8 commits:
Original: v8/v8@03ef3cd
Reland: v8/v8@010897c

Original commit message:
  improve perf_basic_prof filename reporting

  The buffer used for appending filenames to the string printed to the
  perf_basic_prof log was unnecessarily too small. Bump it up to be at least
  kUtf8BufferSize.

  Truncation of filenames makes it really hard to work with profiles gathered on
  Node.js. Because of the way Node.js works, you can have node module dependencies
  in deeply nested directories. The last thing you want when investigating a
  performance problem is to have script names be truncated.

  This patch is a stop-gap. Ideally, I want no truncation of the filename at all
  and use a dynamically growing buffer. That would be a larger change, and I
  wanted to have a quick fix available that can be back-ported to Node.js LTS
  release.

  R=yangguo@chromium.org,yurys@chromium.org
  BUG=

  Review URL: https://codereview.chromium.org/1388543002

PR-URL: #3520
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
ofrobots added a commit that referenced this pull request Oct 28, 2015
This is a reland of #3165. The patch abates
the truncation of script filenames in the perf-event output produced by V8.

V8 commits:
Original: v8/v8@03ef3cd
Reland: v8/v8@010897c

Original commit message:
  improve perf_basic_prof filename reporting

  The buffer used for appending filenames to the string printed to the
  perf_basic_prof log was unnecessarily too small. Bump it up to be at least
  kUtf8BufferSize.

  Truncation of filenames makes it really hard to work with profiles gathered on
  Node.js. Because of the way Node.js works, you can have node module dependencies
  in deeply nested directories. The last thing you want when investigating a
  performance problem is to have script names be truncated.

  This patch is a stop-gap. Ideally, I want no truncation of the filename at all
  and use a dynamically growing buffer. That would be a larger change, and I
  wanted to have a quick fix available that can be back-ported to Node.js LTS
  release.

  R=yangguo@chromium.org,yurys@chromium.org
  BUG=

  Review URL: https://codereview.chromium.org/1388543002

PR-URL: #3520
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
rvagg pushed a commit to rvagg/io.js that referenced this pull request Oct 29, 2015
This is a reland of nodejs#3165. The patch abates
the truncation of script filenames in the perf-event output produced by V8.

V8 commits:
Original: v8/v8@03ef3cd
Reland: v8/v8@010897c

Original commit message:
  improve perf_basic_prof filename reporting

  The buffer used for appending filenames to the string printed to the
  perf_basic_prof log was unnecessarily too small. Bump it up to be at least
  kUtf8BufferSize.

  Truncation of filenames makes it really hard to work with profiles gathered on
  Node.js. Because of the way Node.js works, you can have node module dependencies
  in deeply nested directories. The last thing you want when investigating a
  performance problem is to have script names be truncated.

  This patch is a stop-gap. Ideally, I want no truncation of the filename at all
  and use a dynamically growing buffer. That would be a larger change, and I
  wanted to have a quick fix available that can be back-ported to Node.js LTS
  release.

  R=yangguo@chromium.org,yurys@chromium.org
  BUG=

  Review URL: https://codereview.chromium.org/1388543002

PR-URL: nodejs#3520
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
ofrobots added a commit that referenced this pull request Oct 29, 2015
This is a reland of #3165. The patch abates
the truncation of script filenames in the perf-event output produced by V8.

V8 commits:
Original: v8/v8@03ef3cd
Reland: v8/v8@010897c

Original commit message:
  improve perf_basic_prof filename reporting

  The buffer used for appending filenames to the string printed to the
  perf_basic_prof log was unnecessarily too small. Bump it up to be at least
  kUtf8BufferSize.

  Truncation of filenames makes it really hard to work with profiles gathered on
  Node.js. Because of the way Node.js works, you can have node module dependencies
  in deeply nested directories. The last thing you want when investigating a
  performance problem is to have script names be truncated.

  This patch is a stop-gap. Ideally, I want no truncation of the filename at all
  and use a dynamically growing buffer. That would be a larger change, and I
  wanted to have a quick fix available that can be back-ported to Node.js LTS
  release.

  R=yangguo@chromium.org,yurys@chromium.org
  BUG=

  Review URL: https://codereview.chromium.org/1388543002

PR-URL: #3520
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
@rvagg rvagg mentioned this pull request Dec 17, 2015
@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

@ofrobots @bnoordhuis ... does this need to go into v4

@jasnell
Copy link
Member

jasnell commented Dec 17, 2015

Nevermind... just saw #3237

@bnoordhuis
Copy link
Member

For posterity, #3520 is the second attempt and that landed in v4.x-staging in a6469e9 from October.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants