src: separate trace_event_common from trace_event by targos · Pull Request #10628 · nodejs/node · GitHub

Currently, trace_event.h contains a full copy of the header present in
deps/v8/base/trace_event/common/trace_event_common.h mixed with
implementation-specific macros.
This patch moves the V8 code to its own file and includes it from
trace_event.h.

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

src

/cc @jasongin @nodejs/diagnostics @nodejs/v8

I'm happy to remove the copy and directly include it from deps if you want.

LGTM, certainly an improvement, though I don't know if it's the best approach.

@ofrobots do you know why trace_event_common.h was originally added at deps\v8\trace_event\common rather than deps\v8\include? If it was in the latter location then it would be easier to just #include in trace_event.h here.

Ideally @matthewloring should also comment on this, but I don't know when he is going to be back at work.

IIRC it was quite intentional that @matthewloring did not directly include trace_event_common.h – but the details escape me. Perhaps @fmeawad knows too. Matt should be back mid next week, he has the full context here.

Perhaps we can go with #10623 as the workaround for now and figure out a longer-term solution (this PR or otherwise) once @matthewloring is back.

#10623 is not a workaround for this issue, it is just a fix for a separate tracing-related bug.

Ah sorry, I misunderstood. In that case the work-around might be to copy over the necessary changes from trace_event_common.h in order to unblock #9618? @targos, I can put that together if you want, but it might be simpler for you to add the changes in #9618.

This change is definitely a step in the right direction. The reason I did not depend on the file directly was to avoid a dependency on a directory outside of deps/v8/include. IIRC the file was put outside of deps/v8/include because it was originally copied from Chromium but @fmeawad would have more context.

V8 includes trace_event_common.h here with #include "base/trace_event/common/trace_event_common.h", which relies on the upstream src/base/trace_event/common repo in Chromium here.

@ofrobots @fmeawad would it be a good idea for Node to include the "trace_event_common" repo from Chromium directly under deps here in Node, rather than copy the file to deps/v8 as currently? And long term should we rely on one of these upstreams or our own fork as in #9304 and here?

Conclusion in discussion in Diag WG meeting today is that Node should manage its copy of the trace_event headers however we see fit and not worry about upstream Chromium. V8/Google plan to provide a proper tracing library eventually and when that's ready we can sync with it upstream.

@joshgav thanks. Does it mean that we should go on with this PR?

Currently, trace_event.h contains a full copy of the header present in deps/v8/base/trace_event/common/trace_event_common.h mixed with implementation-specific macros. This patch moves the V8 code to its own file and includes it from trace_event.h. The header is also updated to the latest version from V8 5.6.

Rebased and updated to the latest version from V8 5.6. src/tracing/trace_event_common.h is now a copy of deps/v8/base/trace_event/common/trace_event_common.h.

Any updates on this one?

I'm just waiting for some reviews

This file doesn't need include guards?

@bnoordhuis It has this ^

Right, but the file you moved that from has traditional include guards. I wondered (and wonder) if there is a reason not to add them here.

I didn't think about that. This is just a copy of the file from V8. Would you like me to add include guards?

LGTM.

But it doesn't seem to fix the break noted here (comment), per my testing with this follow-on commit.

cc @matthewloring

There are likely more changes required to account for updates to the macros in V8 5.7 which just landed on master. I'm currently working to put together a set of steps for updating these macros when V8 is updated. I also have a patch that will fix these files for 5.7. I can post the steps I've been using here or open the patch separately. @targos What would you prefer?

@matthewloring go ahead and open a PR

Opened here: #12127

Thanks. I'll close this one then.

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v7.x labels Jan 5, 2017

mscdex removed the dont-land-on-v7.x label Jan 11, 2017

targos added dont-land-on-v4.x labels Jan 28, 2017

mscdex added the trace_events Issues and PRs related to V8, Node.js core, and userspace code trace events. label Jan 29, 2017

joshgav mentioned this pull request Feb 6, 2017

[trace_event] tracking issue nodejs/diagnostics#84

Open

12 tasks

joshgav added the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Feb 23, 2017

joshgav mentioned this pull request Feb 23, 2017

Diag WG Meeting - 2017-02 nodejs/diagnostics#85

Closed

joshgav removed the diag-agenda Issues and PRs to discuss during the meetings of the diagnostics working group. label Feb 23, 2017

targos force-pushed the trace-event-common branch from fde6e01 to f09c2bc Compare March 1, 2017 21:29

bnoordhuis reviewed Mar 26, 2017

View reviewed changes

targos commented Mar 26, 2017

View reviewed changes

joshgav approved these changes Mar 28, 2017

View reviewed changes

targos closed this Mar 30, 2017

targos deleted the trace-event-common branch June 1, 2017 08:02

		@@ -0,0 +1,1073 @@
		// Copyright 2015 The Chromium Authors. All rights reserved.

+              #if defined(TRACE_EVENT0)
+              #error "Another copy of this file has already been included."
+              #endif

src: separate trace_event_common from trace_event by targos · Pull Request #10628 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: separate trace_event_common from trace_event - #10628

src: separate trace_event_common from trace_event#10628targos wants to merge 1 commit intonodejs:masternodejs/node:masterfrom targos:trace-event-commontargos/node:trace-event-commonCopy head branch name to clipboard

Conversation

targos commented Jan 5, 2017

Uh oh!

targos commented Jan 5, 2017

Uh oh!

jasongin commented Jan 5, 2017

Uh oh!

ofrobots commented Jan 5, 2017

Uh oh!

jasongin commented Jan 5, 2017

Uh oh!

ofrobots commented Jan 5, 2017

Uh oh!

matthewloring commented Jan 6, 2017

Uh oh!

joshgav commented Feb 6, 2017

Uh oh!

joshgav commented Feb 23, 2017

Uh oh!

targos commented Feb 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

targos commented Mar 1, 2017

Uh oh!

jasnell commented Mar 24, 2017

Uh oh!

targos commented Mar 25, 2017

Uh oh!

bnoordhuis Mar 26, 2017

Choose a reason for hiding this comment

Uh oh!

targos Mar 26, 2017

Choose a reason for hiding this comment

Uh oh!

bnoordhuis Mar 26, 2017

Choose a reason for hiding this comment

Uh oh!

targos Mar 29, 2017

Choose a reason for hiding this comment

Uh oh!

joshgav commented Mar 28, 2017

Uh oh!

matthewloring commented Mar 29, 2017

Uh oh!

targos commented Mar 29, 2017

Uh oh!

matthewloring commented Mar 29, 2017

Uh oh!

targos commented Mar 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

src: separate trace_event_common from trace_event#10628
targos wants to merge 1 commit into
nodejs:masterfrom
targos:trace-event-common

targos commented Feb 28, 2017 •
edited
Loading