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: update trace event macros to v8 6.3 version #17640

Closed
wants to merge 2 commits into from
Closed

src: update trace event macros to v8 6.3 version #17640

wants to merge 2 commits into from

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Dec 13, 2017 •
edited
Loading

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src, tracing

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. labels Dec 13, 2017
@kjin
Copy link
Contributor Author

kjin commented Dec 13, 2017

cc @ofrobots

@addaleax
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-commit/14786/

@jasnell
Copy link
Member

jasnell commented Dec 13, 2017

Out of curiosity, does this give us support for the WITH_TIMESTAMP macros now?

@BridgeAR
Copy link
Member

I mark it as ready but I think it should probably wait the full 48 hours.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
@kjin
Copy link
Contributor Author

kjin commented Dec 13, 2017 •
edited
Loading

@jasnell I made a mistake here, TIMESTAMP macros are not implemented in v8 yet. I'm checking to see what the status of this is... will provide an update. @BridgeAR can you remove the ready label?

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017
@ofrobots
Copy link
Contributor

I think this PR re-syncs the trace-event implementation with what we have in V8. WITH_TIMESTAMP is not implemented in V8 just yet. Chromium does have it, but it depends on the base C++ headers and atomics that are available in Chromium but not in V8. I can take a look at how to implement WITH_TIMESTAMP as a follow-on – whether they need to be in V8 (and re-synced here) or whether we can add them to Node directly.

@jasnell
Copy link
Member

jasnell commented Dec 14, 2017

No worries and no rush. Was just curious. 😎

mhdawson
mhdawson approved these changes Jan 3, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM assuming CI is green.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 19, 2018
@BridgeAR
Copy link
Member

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

@maclover7 maclover7 force-pushed the master branch 2 times, most recently from bb5575a to 993b716 Compare January 26, 2018 22:02
@BridgeAR
Copy link
Member

New CI again https://ci.nodejs.org/job/node-test-pull-request/12834/

@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

CI is super red: see e.g. https://ci.nodejs.org/job/node-test-commit-freebsd/15082/nodes=freebsd11-x64/console

@kjin would you please have a look at it?

@BridgeAR BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 7, 2018

Ping @kjin

@kjin
Copy link
Contributor Author

kjin commented Feb 13, 2018

@BridgeAR Fixed, sorry about the delay!

@ofrobots
Copy link
Contributor

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

@jasnell
Copy link
Member

jasnell commented Feb 14, 2018

Several errors in CI, none appear to be related.

Rerunning CI on SmartOS just to be safe: https://ci.nodejs.org/job/node-test-commit-smartos/15393/

jasnell pushed a commit that referenced this pull request Feb 14, 2018
@kjin @jasnell
src: update trace event macros to v8 6.4 version
d8ec49e 
PR-URL: #17640
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell
Copy link
Member

jasnell commented Feb 14, 2018

Landed in d8ec49e

@jasnell jasnell closed this Feb 14, 2018
@targos targos mentioned this pull request Apr 2, 2018
Closed
4 tasks
MayaLekova pushed a commit to MayaLekova/node that referenced this pull request May 8, 2018
@kjin @MayaLekova
src: update trace event macros to v8 6.4 version
4f0f789 
PR-URL: nodejs#17640
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@kjin @addaleax @jasnell @BridgeAR @ofrobots @cjihrig @mhdawson @MylesBorins @nodejs-github-bot