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

trace_events: fix dynamic enabling of trace events #22114

Closed
wants to merge 2 commits into from
Closed

trace_events: fix dynamic enabling of trace events #22114

wants to merge 2 commits into from

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Aug 3, 2018 •
edited
Loading

V8 was assuming that trace-events can only be enabled at startup and, once disabled, will never be enabled again. In Node.js we have a lot more dynamic control over tracing. Fix bug upstream.

This PR back ports the fix and adds a test. This can be floated until (if) this is merged upstream.

/cc @nodejs/trace-events @nodejs/v8-update

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

V8-CI: https://ci.nodejs.org/view/All/job/node-test-commit-v8-linux/1570/
CI: https://ci.nodejs.org/job/node-test-pull-request/16180/

@nodejs-github-bot
Copy link
Collaborator

@ofrobots build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/419/pipeline

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Aug 3, 2018
@targos
Copy link
Member

targos commented Aug 4, 2018

Embedder string was bumped twice :)

@ofrobots ofrobots force-pushed the trace-event-dynamic branch from c154d6f to d77cd10 Compare August 8, 2018 12:30
@ofrobots ofrobots mentioned this pull request Aug 9, 2018
Closed
@ofrobots @targos
deps: cherry-pick bf5ea81 from upstream V8
06b083c 
Original commit message:

    [tracing] allow dynamic control of tracing

    If the trace_buffer_ was null, we were returning a pointer to a static
    flag back that permanently disabled that particular trace point.

    This implied an assumption that tracing will be statically enabled at
    process startup, and once it is disabled, it will never be enabled
    again. On Node.js side we want to dynamically enable/disable tracing as per
    programmer intent.

    Change-Id: Ic7a7839b8450ab5c356d85e8e0826f42824907f4
    Reviewed-on: https://chromium-review.googlesource.com/1161518
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Ali Ijaz Sheikh <ofrobots@google.com>
    Cr-Commit-Position: refs/heads/master@{nodejs#54903}

Refs: v8/v8@bf5ea81
@ofrobots @targos
[squash] add test to dynamic enablement of trace-events
4dff389
@targos
Copy link
Member

targos commented Sep 3, 2018 •
edited
Loading

I rebased the PR on top of master.
New CI: https://ci.nodejs.org/job/node-test-pull-request/16995/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1651/

Edit: resumed CI: https://ci.nodejs.org/job/node-test-pull-request/17002/

@targos targos force-pushed the trace-event-dynamic branch from d77cd10 to 4dff389 Compare September 3, 2018 18:05
@targos
Copy link
Member

targos commented Sep 4, 2018

Fixed V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1655/

@targos
Copy link
Member

targos commented Sep 4, 2018

Landed in ec285c8 and 8e91215

@targos targos closed this Sep 4, 2018
targos pushed a commit that referenced this pull request Sep 4, 2018
@ofrobots @targos
deps: cherry-pick bf5ea81 from upstream V8
ec285c8 
Original commit message:

    [tracing] allow dynamic control of tracing

    If the trace_buffer_ was null, we were returning a pointer to a static
    flag back that permanently disabled that particular trace point.

    This implied an assumption that tracing will be statically enabled at
    process startup, and once it is disabled, it will never be enabled
    again. On Node.js side we want to dynamically enable/disable tracing as per
    programmer intent.

    Change-Id: Ic7a7839b8450ab5c356d85e8e0826f42824907f4
    Reviewed-on: https://chromium-review.googlesource.com/1161518
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Ali Ijaz Sheikh <ofrobots@google.com>
    Cr-Commit-Position: refs/heads/master@{#54903}

Refs: v8/v8@bf5ea81

PR-URL: #22114
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 4, 2018
@ofrobots @targos
test: add test to dynamic enablement of trace-events
8e91215 
PR-URL: #22114
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 5, 2018
@ofrobots @targos
deps: cherry-pick bf5ea81 from upstream V8
c84c27f 
Original commit message:

    [tracing] allow dynamic control of tracing

    If the trace_buffer_ was null, we were returning a pointer to a static
    flag back that permanently disabled that particular trace point.

    This implied an assumption that tracing will be statically enabled at
    process startup, and once it is disabled, it will never be enabled
    again. On Node.js side we want to dynamically enable/disable tracing as per
    programmer intent.

    Change-Id: Ic7a7839b8450ab5c356d85e8e0826f42824907f4
    Reviewed-on: https://chromium-review.googlesource.com/1161518
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Ali Ijaz Sheikh <ofrobots@google.com>
    Cr-Commit-Position: refs/heads/master@{#54903}

Refs: v8/v8@bf5ea81

PR-URL: #22114
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 5, 2018
@ofrobots @targos
test: add test to dynamic enablement of trace-events
e5b732f 
PR-URL: #22114
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Sep 5, 2018
Merged
Closed
targos pushed a commit that referenced this pull request Sep 6, 2018
@ofrobots @targos
deps: cherry-pick bf5ea81 from upstream V8
3531aa0 
Original commit message:

    [tracing] allow dynamic control of tracing

    If the trace_buffer_ was null, we were returning a pointer to a static
    flag back that permanently disabled that particular trace point.

    This implied an assumption that tracing will be statically enabled at
    process startup, and once it is disabled, it will never be enabled
    again. On Node.js side we want to dynamically enable/disable tracing as per
    programmer intent.

    Change-Id: Ic7a7839b8450ab5c356d85e8e0826f42824907f4
    Reviewed-on: https://chromium-review.googlesource.com/1161518
    Reviewed-by: Yang Guo <yangguo@chromium.org>
    Commit-Queue: Ali Ijaz Sheikh <ofrobots@google.com>
    Cr-Commit-Position: refs/heads/master@{#54903}

Refs: v8/v8@bf5ea81

PR-URL: #22114
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Sep 6, 2018
@ofrobots @targos
test: add test to dynamic enablement of trace-events
4c34302 
PR-URL: #22114
Reviewed-By: James M Snell <jasnell@gmail.com>
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. 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.
4 participants
@ofrobots @nodejs-github-bot @targos @jasnell