Skip to content
This repository was archived by the owner on Apr 22, 2023. It is now read-only.

deps: log V8 version in profiler log file#9043

Closed
bnoordhuis wants to merge 1 commit intonodejs:v0.10from
bnoordhuis:v8-log-version
Closed

deps: log V8 version in profiler log file#9043
bnoordhuis wants to merge 1 commit intonodejs:v0.10from
bnoordhuis:v8-log-version

Conversation

@bnoordhuis
Copy link
Copy Markdown
Member

Patch from issue 800293002 authored by ben@strongloop.com

TBR=yangguo@chromium.org

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

R=@trevnorris

Patch from issue 800293002 authored by ben@strongloop.com

TBR=yangguo@chromium.org

Review URL: https://codereview.chromium.org/806143002
@bnoordhuis
Copy link
Copy Markdown
Member Author

I can take care of forward-porting it to v0.11.

@trevnorris
Copy link
Copy Markdown

LGTM

@misterdjules @chrisdickinson Have any issues w/ this?

@bnoordhuis
Copy link
Copy Markdown
Member Author

Ping. I'd interpret the lack of replies as an implicit 'yes' (edit: or rather, 'no, no issues'.)

@misterdjules
Copy link
Copy Markdown

@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!

@misterdjules
Copy link
Copy Markdown

@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!

@misterdjules
Copy link
Copy Markdown

@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, d8 doesn't seem to build out of the box. On Linux, I always have to patch the generated makefiles and invoking linux-tick-processor directly segfaults, so I have to to use cat v8.log | deps/v8/out/native/d8 ./deps/v8/tools/splaytree.js ./deps/v8/tools/codemap.js ./deps/v8/tools/csvparser.js ./deps/v8/tools/consarray.js ./deps/v8/tools/profile.js ./deps/v8/tools/profile_view.js ./deps/v8/tools/logreader.js ./deps/v8/tools/tickprocessor.js ./deps/v8/tools/tickprocessor-driver.js.

Am I missing something, or is it just the current state of the tools around profiling in v0.10?

@bnoordhuis
Copy link
Copy Markdown
Member Author

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?

Yes, exactly. If a tool doesn't know the version, it can't select the proper tick processor. The format changes quite often.

Also, why isn't the test from the original upstream change back ported too?

I couldn't get the V8 C++ test suite in v0.10 to build, last time I tried it.

is it just the current state of the tools around profiling in v0.10?

In a word: yes.

@tjfontaine
Copy link
Copy Markdown

Since this is in upstream I'm fine with the change, LGTM

@misterdjules
Copy link
Copy Markdown

@bnoordhuis Thank you for your response, the change LGTM.

trevnorris pushed a commit that referenced this pull request Feb 4, 2015
Patch from issue 800293002 authored by ben@strongloop.com

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

PR-URL: #9043
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Timothy J Fontaine <tjfontaine@gmail.com>
@trevnorris
Copy link
Copy Markdown

Thanks. Landed in 431eb17.

@trevnorris trevnorris closed this Feb 4, 2015
@misterdjules
Copy link
Copy Markdown

Also, just for your information, @jasnell plans to work on making V8's tests suite run with make test soon.

@misterdjules
Copy link
Copy Markdown

@trevnorris Do you want to take care of merging this in v0.12 soon, or should I create an issue to track that?

@trevnorris
Copy link
Copy Markdown

@misterdjules Go ahead and create an issue and assign it to me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants