Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(132)

Issue 1388543002: improve perf_basic_prof filename reporting (Closed)

Created:
5 years, 2 months ago by ofrobots
Modified:
5 years, 2 months ago
Reviewers:
Yang, yurys
CC:
v8-reviews_googlegroups.com
Base URL:
https://chromium.googlesource.com/v8/v8.git@master
Target Ref:
refs/pending/heads/master
Project:
v8
Visibility:
Public.

Description

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= Committed: https://crrev.com/03ef3cd004c2fd31ae7e48772f106df67b8c2feb Cr-Commit-Position: refs/heads/master@{#31092}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -1 line) Patch
M src/log.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 10 (1 generated)
ofrobots
5 years, 2 months ago (2015-10-02 22:37:11 UTC) #1
ofrobots
On 2015/10/02 22:37:11, ofrobots wrote: Here's an example of the entries we are currently getting ...
5 years, 2 months ago (2015-10-02 22:40:29 UTC) #2
yurys
lgtm
5 years, 2 months ago (2015-10-02 22:43:47 UTC) #3
Yang
lgtm
5 years, 2 months ago (2015-10-03 03:05:54 UTC) #4
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1388543002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1388543002/1
5 years, 2 months ago (2015-10-03 05:08:48 UTC) #6
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 2 months ago (2015-10-03 05:32:51 UTC) #7
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/03ef3cd004c2fd31ae7e48772f106df67b8c2feb Cr-Commit-Position: refs/heads/master@{#31092}
5 years, 2 months ago (2015-10-03 05:33:03 UTC) #8
jrobbins
Test comment.
5 years, 2 months ago (2015-10-06 17:34:51 UTC) #9
Jakob Kummerow
5 years, 2 months ago (2015-10-07 09:45:14 UTC) #10
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1390923004/ by jkummerow@chromium.org.

The reason for reverting is: Suspected to cause crbug.com/539892.

Powered by Google App Engine
This is Rietveld 408576698