feat: allow collecting profiles with more accurate line numbers#69
feat: allow collecting profiles with more accurate line numbers#69nolanmar511 merged 3 commits intogoogle:masterfrom
Conversation
nolanmar511
force-pushed
the
accurate-line-numbers
branch
3 times, most recently
from
4a33bf5 to
ab65cce
Compare
nolanmar511
force-pushed
the
accurate-line-numbers
branch
from
48e5b07 to
77c8929
Compare
| NAN_METHOD(StartProfiling) { | ||
| Local<String> name = info[0].As<String>(); | ||
| if (info.Length() != 2) { | ||
| return Nan::ThrowTypeError("StartProfling must have two arguments."); |
| cpuProfiler->StartProfiling(name, false); | ||
| // Sample counts and timestamps are not used, so we do not need to record | ||
| // samples. | ||
| bool recordSamples = false; |
There was a problem hiding this comment.
Is this supposed to be a const?
| } | ||
|
|
||
| Local<Value> TranslateTimeProfile(const CpuProfile* profile) { | ||
| Local<Value> TranslateTimeProfile(const CpuProfile* profile, bool hasDetailedLines) { |
There was a problem hiding this comment.
"detailed" is an unclear term. Also "has" is not super clear - is this flag requesting something or stating something?
There was a problem hiding this comment.
Switched to using "includeLineInfo" (for starting the profile, when we're requesting line information), and "includedLineInfo" (where it indicates that line info is present).
|
|
||
| // Line-level accurate line information is not available in Node 11 or earlier. | ||
| #if NODE_MODULE_VERSION > NODE_11_0_MODULE_VERSION | ||
| bool includeLineInfo = |
There was a problem hiding this comment.
terminology: what is the difference between "has" used above and "include" used here? Is this intentionally different?
There was a problem hiding this comment.
Switched to using "includeLineInfo" (for starting the profile, when we're requesting line information), and "includedLineInfo" (where it indicates that line info is present).
| } | ||
|
|
||
| async function collectAndSaveTimeProfile(durationSeconds, sourceMapper, | ||
| lineNumbers) { |
There was a problem hiding this comment.
Nit: I am not sure it's the right formatting, I think lineNumbers needs to be aligned with the first argument. Is this machine-formatted?
|
|
||
| if [[ "$VERIFY_TIME_LINE_NUMBERS" == "true" ]]; then | ||
| pprof -lines -top -nodecount=2 time.pb.gz | \ | ||
| grep "busyLoop.*src/busybench.js:33" |
There was a problem hiding this comment.
I would change ":33" to ":34" here once and make sure the test fails, just to double-check that the "verify time line numbers" flag machinery works correctly. As if it doesn't the test would happily pass without the line numbers feature working.
|
|
||
| /** | ||
| * This configuration option is experimental. | ||
| * When set to true, functions will be aggregated at the line-level, rather |
There was a problem hiding this comment.
"line-level" -> "line level"? because "function level" (and overall)
4 participants
Testing:
I've run test.sh with Node 12.3.1 and the V8 canary build from 6/10/19 (with and without VERIFY_TIME_LINE_NUMBERS=true).
Also inspected time profiles from each of these runs:
Node 13, w/o detailed lines
Node 13 w/ detailed lines
Node 12 w/o detailed lines
Node 12 w/ detailed lines
