deps,profiler: parse Windows' "%p" in processor #14510

jaimecbernardo · 2017-07-27T12:30:10Z

When V8 was updated to 5.4, the output format for memory addresses was changed from 0x%x to %p. The %p format specified is implementation specific and behaves differently on Windows.

Running the profiler processor ( --prof-process) with a profiler output file generated on Windows (with --prof) results in "UNKNOWN" code dominating the statistics. This is caused by the processor not correctly parsing the output of the %p format specifier on Windows, which prints 8 (in 32 bits) or 16 (in 64 bits) hexadecimal characters without the 0x prefix. The profiler processor expects a format that is recognized by parseInt.

Example line from profiler output on Linux:

code-creation,Builtin,5,0x12e0553593e0,327,"MathMax"

Example line from profiler output on Windows:

code-creation,Builtin,5,000000BABADD9640,304,"MathMax"

While the 0x12e0553593e0 address string from Linux will be correctly parsed by parseInt in the profiler processor, the 000000BABADD9640 address string from Windows will not.

The commit on this PR creates a thin layer on top of parseInt to parse the Windows %p format for memory addresses correctly.

I've tried building V8 from source on windows to test the profiler behavior using d8 and v8/tools/windows-tick-processor.bat and confirmed this profiler issue is also currently present on Windows in V8's master branch.

Fixes: #8221
Refs: #8317
Refs:
v8/v8@9041833#diff-738e87867268a7b9c90329adc160855cR167

/cc @nodejs/v8

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

deps,v8,win

Running the profiler processor (--prof-process) with a profiler output file generated on Windows (with --prof) results in "UNKNOWN" code dominating the statistics. This is caused by the processor not correctly parsing the output of the "%p" format specifier on Windows, which prints 8 (in 32 bits) or 16 (in 64 bits) hexadecimal characters without the "0x" prefix. The profiler processor expects a format that is recognized by parseInt. This commit creates a thin layer on top of parseInt to parse the Windows "%p" format for memory addresses correctly. Fixes: nodejs#8221 Refs: v8/v8@9041833#diff-738e87867268a7b9c90329adc160855cR167

addaleax · 2017-07-27T12:32:13Z

deps/v8/tools/tickprocessor.js

+ */
+TickProcessor.prototype.parseIntAddressFormat = function(s, radix) {
+  var re = /^([0-9A-Fa-f]{8}|[0-9A-Fa-f]{16})$/g
+  if(s.match(re)) {


tiny nit: space after if

Thank you! Added a fixup commit.

addaleax · 2017-07-27T12:32:38Z

deps/v8/tools/tickprocessor.js

+  if(s.match(re)) {
+    return parseInt(s,16)
+  } else {
+    return parseInt(s,radix)


again, nits: space after the , here and in the if branch

Thank you! Added a fixup commit.

XadillaX

LGTM if CI is green.

XadillaX · 2017-07-27T12:57:29Z

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

addaleax · 2017-07-27T13:00:20Z

Sorry, I’m just now realizing that this only changes files in deps/v8…

Did you try upstreaming these changes to V8? We usually aren’t very keen on floating our own patches. Also, we’d be glad to help you bring this patch into V8, I know it can be a bit difficult to get used to their process.

jaimecbernardo · 2017-07-27T13:20:38Z

@addaleax I'm preparing to upstream the change to V8 as well.

I decided to open a PR here first since it was ready and the changes were in response to #8221 , so the current issue can also be understood. I also took into account that the current V8 in node/master is 5.8 and that V8 upstream seems to be in major version 6 already, so the adoption of the patch could take a while.

Thanks for your help. I've already made a change upstream once.
I'll post a link here once I've sent this upstream as well.

addaleax · 2017-07-27T13:24:58Z

I also took into account that the current V8 in node/master is 5.8 and that V8 upstream seems to be in major version 6 already, so the adoption of the patch could take a while.

Just in case it helps avoid confusion, V8 doesn’t use semver and major versions go 5.8, 5.9, 6.0, 6.1, etc. without a huge difference between 5.9 and 6.0. We’re actually using V8 5.9 in master already and are hoping to upgrade to V8 6.0 until next week. Also, floating patches gets a bit easier/meets less resistance when it’s a cherry-pick from upstream that we don’t have to manage ourselves.

Trott · 2017-07-27T21:06:11Z

So this should be closed and redirected to the V8 repository? And re-opened if V8 rejects it but we decide we want/need it?

Trott · 2017-07-27T21:06:37Z

(Adding blocked label based on my above comment, but feel free to remove it or request that it be removed if I'm misguided here.)

addaleax · 2017-07-27T21:08:53Z

We can leave this PR open until there’s some upstream decision and turn it into a cherry-pick PR if we like. We can also close it for now, I guess, as long as we don’t forget about it.

jaimecbernardo · 2017-07-28T12:01:13Z

I've submitted it upstream here:
https://chromium-review.googlesource.com/c/590234/

Added @addaleax in CC as well.

jaimecbernardo · 2017-07-28T15:05:50Z

Another change was made upstream to make the ouput from the profiler be the same in Windows. https://chromium-review.googlesource.com/c/591373
It seems to have landed here: https://chromium.googlesource.com/v8/v8/+/4229ca207e1d139d57cd9c2e72c6174d4c81878c

So, I guess this PR can be discarded.

tniessen · 2017-07-29T10:34:29Z

Yes, this was fixed upstream in v8/v8@4229ca2. Feel free to open a cherry-pick PR.

nodejs-github-bot added the v8 engine Issues and PRs related to the V8 dependency. label Jul 27, 2017

addaleax approved these changes Jul 27, 2017

View reviewed changes

jaimecbernardo mentioned this pull request Jul 27, 2017

32bit address given for 64bit builds using --prof on Windows #8221

Closed

fixup: add spaces to conform to JS style

ffc33fb

XadillaX approved these changes Jul 27, 2017

View reviewed changes

Trott added the blocked PRs that are blocked by other issues or PRs. label Jul 27, 2017

tniessen closed this Jul 29, 2017

jaimecbernardo mentioned this pull request Jul 29, 2017

deps: cherry-pick 4229ca2 from V8 upstream #14538

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

deps,profiler: parse Windows' "%p" in processor #14510

deps,profiler: parse Windows' "%p" in processor #14510

jaimecbernardo commented Jul 27, 2017

addaleax Jul 27, 2017

jaimecbernardo Jul 27, 2017

addaleax Jul 27, 2017

jaimecbernardo Jul 27, 2017

XadillaX left a comment •

edited

Loading

XadillaX commented Jul 27, 2017

addaleax commented Jul 27, 2017

jaimecbernardo commented Jul 27, 2017

addaleax commented Jul 27, 2017

Trott commented Jul 27, 2017

Trott commented Jul 27, 2017

addaleax commented Jul 27, 2017

jaimecbernardo commented Jul 28, 2017

jaimecbernardo commented Jul 28, 2017

tniessen commented Jul 29, 2017

deps,profiler: parse Windows' "%p" in processor #14510

deps,profiler: parse Windows' "%p" in processor #14510

Conversation

jaimecbernardo commented Jul 27, 2017

Checklist

Affected core subsystem(s)

addaleax Jul 27, 2017

Choose a reason for hiding this comment

jaimecbernardo Jul 27, 2017

Choose a reason for hiding this comment

addaleax Jul 27, 2017

Choose a reason for hiding this comment

jaimecbernardo Jul 27, 2017

Choose a reason for hiding this comment

XadillaX left a comment • edited Loading

Choose a reason for hiding this comment

XadillaX commented Jul 27, 2017

addaleax commented Jul 27, 2017

jaimecbernardo commented Jul 27, 2017

addaleax commented Jul 27, 2017

Trott commented Jul 27, 2017

Trott commented Jul 27, 2017

addaleax commented Jul 27, 2017

jaimecbernardo commented Jul 28, 2017

jaimecbernardo commented Jul 28, 2017

tniessen commented Jul 29, 2017

XadillaX left a comment •

edited

Loading