deps: log V8 version in profiler log file #9043
Conversation
Patch from issue 800293002 authored by [email protected] [email protected] Review URL: https://codereview.chromium.org/806143002
|
I can take care of forward-porting it to v0.11. |
|
LGTM @misterdjules @chrisdickinson Have any issues w/ this? |
|
Ping. I'd interpret the lack of replies as an implicit 'yes' (edit: or rather, 'no, no issues'.) |
|
@bnoordhuis Sorry for lagging behind :( The lack of reply on my end is more related to being busy with a lot of other stuff. I'll try to take a closer look at it today. Thanks! |
|
@bnoordhuis The functionality definitely seem useful. However, Node.js doesn't upgrade V8 that often, and it's frozen in v0.10 so I'm not sure I fully understand the use case for having that landed in v0.10. Is it so that when dealing with a lot of v8 logs from different Node.js/io.js versions, including node v0.10.x, it's easier to determine which profile log is from which V8/Node.js/io.js version? Also, why isn't the test from the original upstream change back ported too? The version is not outputted by the tick processor, is it because it's not needed or that might come later? Thank you! |
|
@bnoordhuis Also, this is unrelated to the change itself, but I always have a hard time running V8's profiler on OS X and Linux. On OS X, Am I missing something, or is it just the current state of the tools around profiling in v0.10? |
Yes, exactly. If a tool doesn't know the version, it can't select the proper tick processor. The format changes quite often.
I couldn't get the V8 C++ test suite in v0.10 to build, last time I tried it.
In a word: yes. |
|
Since this is in upstream I'm fine with the change, LGTM |
|
@bnoordhuis Thank you for your response, the change LGTM. |
Patch from issue 800293002 authored by [email protected] Review URL: https://codereview.chromium.org/806143002 PR-URL: #9043 Reviewed-by: Trevor Norris <[email protected]> Reviewed-By: Timothy J Fontaine <[email protected]>
|
Thanks. Landed in 431eb17. |
|
Also, just for your information, @jasnell plans to work on making V8's tests suite run with |
|
@trevnorris Do you want to take care of merging this in v0.12 soon, or should I create an issue to track that? |
|
@misterdjules Go ahead and create an issue and assign it to me. |
Patch from issue 800293002 authored by [email protected] Review URL: https://codereview.chromium.org/806143002 PR-URL: nodejs/node-v0.x-archive#9043 Reviewed-by: Trevor Norris <[email protected]> Reviewed-By: Timothy J Fontaine <[email protected]> Signed-off-by: Jeroen Ooms <[email protected]>
Patch from issue 800293002 authored by [email protected]
TBR=[email protected]
Review URL: https://codereview.chromium.org/806143002
R=@trevnorris