Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

v8: GetCpuProfiler is going away soon #18039

Closed
targos opened this issue Jan 8, 2018 · 5 comments
Closed

v8: GetCpuProfiler is going away soon #18039

targos opened this issue Jan 8, 2018 · 5 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.

Comments

@targos
Copy link
Member

targos commented Jan 8, 2018

A new API was introduced in v8/v8@120b753.
The current API is deprecated in V8 6.4: v8/v8@8c5e2d7

Our usage:

node/src/env.cc

Lines 150 to 160 in 6aac05b

void Environment::StartProfilerIdleNotifier() {
uv_prepare_start(&idle_prepare_handle_, [](uv_prepare_t* handle) {
Environment* env = ContainerOf(&Environment::idle_prepare_handle_, handle);
env->isolate()->GetCpuProfiler()->SetIdle(true);
});
uv_check_start(&idle_check_handle_, [](uv_check_t* handle) {
Environment* env = ContainerOf(&Environment::idle_check_handle_, handle);
env->isolate()->GetCpuProfiler()->SetIdle(false);
});
}

@targos targos added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Jan 8, 2018
@targos
Copy link
Member Author

targos commented Jan 8, 2018

/cc @nodejs/v8

@ofrobots
Copy link
Contributor

ofrobots commented Jan 8, 2018

Hmm.. with the new API it seems that it should be possible to create multiple instances of the CPU profiler. Previously it was a singleton. This means that user-space modules could all use different profilers, and the idle notification above would stop working. Does this mean that Node needs to instantiate a singleton profiler and the ecosystem needs to change to start using that?

/cc @hashseed @fhinkel is my understanding correct?

@nolanmar511 nolanmar511 mentioned this issue Jan 8, 2018
Closed
@fhinkel
Copy link
Member

fhinkel commented Jan 9, 2018

Yes, with the old API, every isolate has one profiler assigned, created during Isolate::Init. With the new API, embedders can create multiple profilers.

Node could instantiate a singleton and provide that - or maybe we should invert dependencies: Addons can use as many profilers as they want, but they are responsible for registering for the IdleNotifier.

@targos targos mentioned this issue Jan 26, 2018
Closed
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Feb 2, 2018
@bnoordhuis
src: fix GetCpuProfiler() deprecation warning
8aed1e8 
Replace `v8::Isolate::GetCpuProfiler()` with `v8::CpuProfiler::New()`
and cache the instance; creating and disposing an instance every loop
tick is too expensive.

Fixes: nodejs#18039
@bnoordhuis bnoordhuis mentioned this issue Feb 2, 2018
Closed
@bnoordhuis
Copy link
Member

Node could instantiate a singleton and provide that - or maybe we should invert dependencies: Addons can use as many profilers as they want, but they are responsible for registering for the IdleNotifier.

I don't think we have to worry about that. We only use it to call CpuProfiler::SetIdle() and that's not even a proper CpuProfiler method (it updates v8::internal::Isolate::current_vm_state_.) Add-ons can create their own CpuProfiler instance.

I've opened #18534 with a simple fix.

@bnoordhuis
Copy link
Member

And https://chromium-review.googlesource.com/c/v8/v8/+/900622 to discuss simplifying the API.

@danbev danbev closed this as completed in f02b74d Feb 6, 2018
danbev pushed a commit to danbev/node that referenced this issue Feb 23, 2018
@bnoordhuis @danbev
src: fix GetCpuProfiler() deprecation warning
2a52d46 
Replace `v8::Isolate::GetCpuProfiler()` with `v8::CpuProfiler::New()`
and cache the instance; creating and disposing an instance every loop
tick is too expensive.

PR-URL: nodejs#18534
Fixes: nodejs#18039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@danbev danbev mentioned this issue Feb 23, 2018
Closed
2 tasks
addaleax pushed a commit that referenced this issue Feb 27, 2018
@bnoordhuis @addaleax
src: fix GetCpuProfiler() deprecation warning
e33b9fa 
Replace `v8::Isolate::GetCpuProfiler()` with `v8::CpuProfiler::New()`
and cache the instance; creating and disposing an instance every loop
tick is too expensive.

Backport-PR-URL: #18959
PR-URL: #18534
Fixes: #18039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
kisg pushed a commit to paul99/v8mips that referenced this issue Mar 7, 2018
@hashseed
[cpu-profiler] Move SetIdle() to v8::Isolate
308d4e2 
The VM state is a property of the isolate, not the CPU profiler.
Having to create a v8::CpuProfiler instance in order to change
the property is somewhat inefficient.

See nodejs/node#18039 and
nodejs/node#18534 for context.

Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
Change-Id: I70e31deca6529bccc05a0f4ed500ee268fb63cb8
Reviewed-on: https://chromium-review.googlesource.com/900622
Commit-Queue: Yang Guo <yangguo@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51779}
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
@bnoordhuis @MayaLekova
src: fix GetCpuProfiler() deprecation warning
2f72a5f 
Replace `v8::Isolate::GetCpuProfiler()` with `v8::CpuProfiler::New()`
and cache the instance; creating and disposing an instance every loop
tick is too expensive.

PR-URL: nodejs#18534
Fixes: nodejs#18039
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ofrobots @fhinkel @bnoordhuis @targos