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

src: fix GetCpuProfiler() deprecation warning #18534

Closed
wants to merge 1 commit into from
Closed

src: fix GetCpuProfiler() deprecation warning #18534

wants to merge 1 commit into from

Conversation

bnoordhuis
Copy link
Member

Replace v8::Isolate::GetCpuProfiler() with v8::CpuProfiler::New()
and cache the instance; creating and disposing an instance every loop
tick is too expensive.

Fixes: #18039

@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
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 2, 2018
@bnoordhuis bnoordhuis mentioned this pull request Feb 2, 2018
Closed
@joyeecheung
Copy link
Member

cc @ofrobots @fhinkel

@joyeecheung joyeecheung added the v8 engine Issues and PRs related to the V8 dependency. label Feb 3, 2018
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 3, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 3, 2018

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

@danbev
Copy link
Contributor

danbev commented Feb 6, 2018

node-test-commit failure looks unrelated

console output:

not ok 1898 addons/stringbytes-external-exceed-max/test-stringbytes-external-exceed-max
  ---
  duration_ms: 15.610
  severity: crashed
  stack: |-
    oh no!
    exit code: CRASHED (Signal: 9)
  ...

danbev pushed a commit that referenced this pull request Feb 6, 2018
@bnoordhuis @danbev
src: fix GetCpuProfiler() deprecation warning
f02b74d 
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: #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>
@danbev
Copy link
Contributor

danbev commented Feb 6, 2018

Landed in f02b74d.

@danbev danbev closed this Feb 6, 2018
@MylesBorins
Copy link
Contributor

Should this be backported to v9.x-staging? If yes please follow the guide and raise a backport PR, if not let me know or add the dont-land-on label.

danbev pushed a commit to danbev/node that referenced this pull request 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 pull request Feb 23, 2018
Closed
2 tasks
addaleax pushed a commit that referenced this pull request 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>
@addaleax addaleax mentioned this pull request Feb 27, 2018
Closed
kisg pushed a commit to paul99/v8mips that referenced this pull request 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 pull request 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
Reviewers

@danbev danbev danbev approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gireeshpunathil gireeshpunathil gireeshpunathil approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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

Successfully merging this pull request may close these issues.
10 participants
@bnoordhuis @joyeecheung @BridgeAR @danbev @MylesBorins @jasnell @addaleax @cjihrig @gireeshpunathil @nodejs-github-bot