Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
988893003:
Implement tracing interface for v8 (Closed)
|
Created:
5 years, 9 months ago by fmeawad Modified:
5 years ago Reviewers:
noordhuis, Sven Panne, jochen (gone - plz use gerrit), Yang, caseq, rmcilroy, yurys, dsinclair CC:
alph, marja, Paweł Hajdan Jr., v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionImplement tracing interface for v8
This is based on the Skia Implementation.
More on the project can be found here:
https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4qwiSsDzMkO4tU-Qes/edit#heading=h.p97rw6yt8o2j
The V8 Tracing platform will replace the isolate->event_logger().
But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set.
Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing.
Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation)
BUG=v8:4560
LOG=N
Committed: https://crrev.com/70a7c754bf3445a8b783b75ca2a7aa34cdeb080b
Cr-Commit-Position: refs/heads/master@{#32959}
Patch Set 1 #Patch Set 2 : minor fix #Patch Set 3 : Remove debugging data #Patch Set 4 : Intergate with log-inl #Patch Set 5 : fix variadic arguments. #Patch Set 6 : Add GN build configuration #Patch Set 7 : Minor fix #Patch Set 8 : Another minor #Patch Set 9 : Make sure old embedders still work #
Total comments: 12
Patch Set 10 : Remove calls to event_logger from EventTracer and other review fixes #Patch Set 11 : Add a Unittest for trace-event #
Total comments: 8
Patch Set 12 : Add the use of context_id to store the isolate_id, expand the unit test, and rebase #Patch Set 13 : Minor #
Total comments: 14
Patch Set 14 : Address comments: Implement Atomic macros, Add Disallow Copy.. #
Total comments: 4
Patch Set 15 : Remove v8-tracing in favor of v8-platform, make passing Isolate* explicit, still WIP #
Total comments: 8
Patch Set 16 : Address comments, still WIP #Patch Set 17 : Remove trace-event-common.h and use DEPs instead, remove src/v8.h from trace-event.h, file a bug fo… #
Total comments: 4
Patch Set 18 : Add more comments #Patch Set 19 : Define NULL #Patch Set 20 : Remove pedantic on gcc to allow for zero variadic macro arguments #Patch Set 21 : Rebase #
Total comments: 2
Patch Set 22 : Add trace-event.cc to BUILD.gn #
Total comments: 2
Patch Set 23 : Fix the depedency with chromium issue (patch by eisigner@) #Patch Set 24 : Add default implementation for tracing methods in v8-platform to make roll easier, added include-di… #Patch Set 25 : Update BUILD.gn to include trace_event_common.h #
Messages
Total messages: 88 (28 generated)
fmeawad@chromium.org changed reviewers: + yangguo@chromium.org
PTAL.
yangguo@chromium.org changed reviewers: + jochen@chromium.org, svenpanne@chromium.org
I don't see how we could distinguish events from different isolates. This has been possible in Isolate::SetEventLogger. Now events from different isolates will be mixed together. In case of Worker threads this can become very confusing. Test cases would be great too. Given the scope of this change I'd also like opinions from Sven and Jochen. https://codereview.chromium.org/988893003/diff/160001/include/v8-tracing.h File include/v8-tracing.h (right): https://codereview.chromium.org/988893003/diff/160001/include/v8-tracing.h#ne... include/v8-tracing.h:26: // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. Please use the shorter license header here and elsewhere: // Copyright 2012 the V8 project authors. All rights reserved. // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. https://codereview.chromium.org/988893003/diff/160001/include/v8-tracing.h#ne... include/v8-tracing.h:66: virtual const uint8_t* getCategoryGroupEnabled(const char* name) = 0; Please use all upper case here and below. https://codereview.chromium.org/988893003/diff/160001/include/v8-tracing.h#ne... include/v8-tracing.h:80: static EventTracer* gInstance; V8 doesn't use this naming convention. Just calling it "instance" is fine. https://codereview.chromium.org/988893003/diff/160001/src/tracing/event-trace... File src/tracing/event-tracer.cc (right): https://codereview.chromium.org/988893003/diff/160001/src/tracing/event-trace... src/tracing/event-tracer.cc:68: static void cleanup_tracer() { I'd use CamelCase for those function names. https://codereview.chromium.org/988893003/diff/160001/src/tracing/trace-event.h File src/tracing/trace-event.h (right): https://codereview.chromium.org/988893003/diff/160001/src/tracing/trace-event... src/tracing/trace-event.h:3: // modification, are permitted provided that the following conditions are I assume that this file originates from Chromium? How do we make sure this stays in sync?
It generally seems to me that we are moving some logic from Chromium/Blink into V8 for little gain, but causes some problems (that I already mentioned). Why don't we keep the EventLogger callback and put the bulk of this CL in Blink's V8 bindings, in the LogEventCallback? Do you plan to add more than TRACE_EVENT_BEGIN0 and TRACE_EVENT_END0 in log-inl.h? https://codereview.chromium.org/988893003/diff/160001/src/tracing/event-trace... File src/tracing/event-tracer.cc (right): https://codereview.chromium.org/988893003/diff/160001/src/tracing/event-trace... src/tracing/event-tracer.cc:35: i::Isolate* isolate_ = i::Isolate::Current(); - Do not use Isolate::Current anymore. We are trying to get rid of it. - This use of default init value seems wrong. We would use the same Isolate returned Isolate::Current() at the point of the first call to GetInstance. It's not guaranteed that the isolate is even alive when we run addTraceEvent.
Hi Yang, Thank you for your quick review. I have fixed the isolate issue. The event-tracer implementation was intended to be very temporary as I was trying to simplify a 3-repo (V8/Blink/Chromium) change. But I found that it might not be needed. After this CL lands, the chromium change will go in, which will result duplicate v8 events at the moment. But I will have the blink CL ready as well and try to commit it right after the Chromium CL lands (to remove the duplication). The event-tracer in chromium is going in src/gin and not in Blink The Blink CL will remove the event-logger from the blink side. You also had a concern of why we are implementing this for little to no gain. The idea is to have tracing being used in V8 similar to how it is used in Blink, Skia, and Chromium. Currently, V8 have limited logging/tracing, and we want to expand that. Immediate wins: disabled categories (to be used for IcMisses and some GC logs), Async events and flow events (to be used across threads) as well as adding more arguments to any log entry. The short term plan to to have the CallLogger use it, but eventually we will remove both the CallLogger and the HistogramTimers in favor of tracing. It will also enable us to remove the logging logic from V8 and push into the embedders. (A d8 CL will follow). After this CL lands, any V8 developer can #include trace-event.h and start using the tracing macros. https://codereview.chromium.org/988893003/diff/160001/include/v8-tracing.h File include/v8-tracing.h (right): https://codereview.chromium.org/988893003/diff/160001/include/v8-tracing.h#ne... include/v8-tracing.h:26: // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. On 2015/03/13 07:52:10, Yang wrote: > Please use the shorter license header here and elsewhere: > > // Copyright 2012 the V8 project authors. All rights reserved. > // Use of this source code is governed by a BSD-style license that can be > // found in the LICENSE file. Done. https://codereview.chromium.org/988893003/diff/160001/include/v8-tracing.h#ne... include/v8-tracing.h:66: virtual const uint8_t* getCategoryGroupEnabled(const char* name) = 0; On 2015/03/13 07:52:10, Yang wrote: > Please use all upper case here and below. Done. https://codereview.chromium.org/988893003/diff/160001/include/v8-tracing.h#ne... include/v8-tracing.h:80: static EventTracer* gInstance; On 2015/03/13 07:52:10, Yang wrote: > V8 doesn't use this naming convention. Just calling it "instance" is fine. Done. https://codereview.chromium.org/988893003/diff/160001/src/tracing/event-trace... File src/tracing/event-tracer.cc (right): https://codereview.chromium.org/988893003/diff/160001/src/tracing/event-trace... src/tracing/event-tracer.cc:35: i::Isolate* isolate_ = i::Isolate::Current(); On 2015/03/13 08:07:48, Yang wrote: > - Do not use Isolate::Current anymore. We are trying to get rid of it. > - This use of default init value seems wrong. We would use the same Isolate > returned Isolate::Current() at the point of the first call to GetInstance. It's > not guaranteed that the isolate is even alive when we run addTraceEvent. Done. https://codereview.chromium.org/988893003/diff/160001/src/tracing/event-trace... src/tracing/event-tracer.cc:68: static void cleanup_tracer() { On 2015/03/13 07:52:10, Yang wrote: > I'd use CamelCase for those function names. Done. https://codereview.chromium.org/988893003/diff/160001/src/tracing/trace-event.h File src/tracing/trace-event.h (right): https://codereview.chromium.org/988893003/diff/160001/src/tracing/trace-event... src/tracing/trace-event.h:3: // modification, are permitted provided that the following conditions are On 2015/03/13 07:52:10, Yang wrote: > I assume that this file originates from Chromium? How do we make sure this stays > in sync? It does originate from chromium. At the moment there is no guarantee that it would stay in sync. But there are multiple instance of it in third_parties already (e.g. Blink, Skia, Webrtc, Angle, ...). So it is known to be everywhere, so changing it is limited. There is a plan to make it in its own library, but not in the near future. Even if it gets out of sink (which might happen if V8 or other library have different needs), it will always use the same interface (as defined in v8-tracing.h).
If I'm not mistaken, the next step would be to replace logging in V8 by those TRACE_* macros and then implement the logging backend (a EventTracer instance). However, we have the use case of wanting to produce a V8-specific v8.log when running Chrome for example with --js-flags="--prof". More often than not, in this use case, worker threads run their own isolates, so that we have to use --logfile-per-isolate to prevent events from different isolates to get mixed up. Now that EventTracer does not distinguish between isolates, I don't see how that can be implemented. Is there any alternative way to produce an isolate-specific equivalent to v8.log in Chrome?
with the current integration in blink where we run one isolate per thread, aggregating samples per thread should work, no?
On 2015/03/17 10:47:32, jochen (slow) wrote: > with the current integration in blink where we run one isolate per thread, > aggregating samples per thread should work, no? I'm just concerned that the workflow of running Chrome with --js-flag="--prof --logfile-per-isolate" to use V8's tools for profiling/visualizing will break without adequate replacement.
Hmm! I think this is actually an interesting use case to get right, if possible... I'm open to storing an isolate id or "world id" on every trace event that comes in from v8 or blink. For instance, we could add to base/debug/trace_event* a iid field on each trace event, and figure out how to plumb that into the AddEvent calls. It might be a pain to plumb, but I think at the other end, it would allow trace viewer to show the entire trace by "renderers" instead of right now by thread. This could give us the right long term way to clean up how Devtools timeline figures out which parts of a trace should show up: it right now just assumes all of the CrRendererMain is running tasks for the tab being inspected. But, I think if we could store the isolate id on events, then we'd be working toward a less hacky solution there too... I'm a little unclear if isolate-ids on tasks is "the solution" for devtools use case, or if we want to store something a little more chrome-side specific as the id. But this does seem like a promising avenue... Fadi, wanna brainstorm a little bit about options here in person?
On 2015/03/17 11:07:00, nduca wrote: > Hmm! I think this is actually an interesting use case to get right, if > possible... > > I'm open to storing an isolate id or "world id" on every trace event that comes > in from v8 or blink. For instance, we could add to base/debug/trace_event* a iid > field on each trace event, and figure out how to plumb that into the AddEvent > calls. It might be a pain to plumb, but I think at the other end, it would allow > trace viewer to show the entire trace by "renderers" instead of right now by > thread. > > This could give us the right long term way to clean up how Devtools timeline > figures out which parts of a trace should show up: it right now just assumes all > of the CrRendererMain is running tasks for the tab being inspected. But, I think > if we could store the isolate id on events, then we'd be working toward a less > hacky solution there too... > Nat, can you elaborate on this? I don't see how storing isolate id on trace events would help link events to tabs. All tabs are rendered on the main thread and share one and the same isolate. Each worker runs on its own separate thread which in turn has its own isolate. AFAIU adding isolate id doesn't add new information as we already have tid in each event.
yurys@chromium.org changed reviewers: + yurys@chromium.org
On 2015/03/18 08:56:16, yurys wrote: > On 2015/03/17 11:07:00, nduca wrote: > > Hmm! I think this is actually an interesting use case to get right, if > > possible... > > > > I'm open to storing an isolate id or "world id" on every trace event that > comes > > in from v8 or blink. For instance, we could add to base/debug/trace_event* a > iid > > field on each trace event, and figure out how to plumb that into the AddEvent > > calls. It might be a pain to plumb, but I think at the other end, it would > allow > > trace viewer to show the entire trace by "renderers" instead of right now by > > thread. > > > > This could give us the right long term way to clean up how Devtools timeline > > figures out which parts of a trace should show up: it right now just assumes > all > > of the CrRendererMain is running tasks for the tab being inspected. But, I > think > > if we could store the isolate id on events, then we'd be working toward a less > > hacky solution there too... > > > Nat, can you elaborate on this? > I don't see how storing isolate id on trace events would help link events to > tabs. > All tabs are rendered on the main thread and share one and the same isolate. > Each > worker runs on its own separate thread which in turn has its own isolate. AFAIU > adding > isolate id doesn't add new information as we already have tid in each event. Nat and I had a couple of discussions yesterday, I will write up a doc and share it with everyone on this bug as well as the tracing team. Questions I have, is thread to isolate relation always one-to-one? How about service workers? How about Web workers? Is the optimizing compiler run in the same Isolate? (I know it runs in a different thread)
> Nat and I had a couple of discussions yesterday, > I will write up a doc and share it with everyone on this bug as well as the tracing team. > Questions I have, is thread to isolate relation always one-to-one? in renderer processes, yes. > How about service workers? How about Web workers? a worker runs in a thread in a renderer (each worker has its own thread) > Is the optimizing compiler run in the same Isolate? (I know it runs in a different thread) the optimizing compiler and the GC sweeper run in chromium's thread pool (base::WorkerPool)
On 2015/03/18 18:26:58, jochen (OOO) wrote: > > Nat and I had a couple of discussions yesterday, > > I will write up a doc and share it with everyone on this bug as well as the > tracing team. > > Questions I have, is thread to isolate relation always one-to-one? > > in renderer processes, yes. > > > How about service workers? How about Web workers? > > a worker runs in a thread in a renderer (each worker has its own thread) > > > Is the optimizing compiler run in the same Isolate? (I know it runs in a > different thread) > > the optimizing compiler and the GC sweeper run in chromium's thread pool > (base::WorkerPool) Sorry for the late reply, as follow is a doc with a cid implementation suggestion:https://docs.google.com/a/chromium.org/document/d/1gv6DB8XPhHNk7ld1GK9la49_5WpqQ4d5wP6-VHb4u_I/edit
dsinclair@chromium.org changed reviewers: + dsinclair@chromium.org
I'm guessing you're going to want to use the base/trace_event/trace_event.h instead of the Skia one as it's older then what is in base at this point .... https://codereview.chromium.org/988893003/diff/200001/include/v8-tracing.h File include/v8-tracing.h (right): https://codereview.chromium.org/988893003/diff/200001/include/v8-tracing.h#ne... include/v8-tracing.h:41: }; There is also a ENABLED_FOR_ETW_EXPORT = 1 << 3 should that be here for completeness, or a note that it's skipped on purpose? https://codereview.chromium.org/988893003/diff/200001/src/tracing/trace-event.h File src/tracing/trace-event.h (right): https://codereview.chromium.org/988893003/diff/200001/src/tracing/trace-event... src/tracing/trace-event.h:895: #define TRACE_EVENT_PHASE_INSTANT ('i') This doesn't match the base/trace_event/trace_event.h. It lists Instant as "I" https://codereview.chromium.org/988893003/diff/200001/src/tracing/trace-event... src/tracing/trace-event.h:900: #define TRACE_EVENT_PHASE_FLOW_BEGIN ('s') The NESTABLE_ASYNC_ stuff is missing from here (and I'm assuming the code above). We're phasing out async in favour of nestable async. https://codereview.chromium.org/988893003/diff/200001/src/tracing/trace-event... src/tracing/trace-event.h:909: PHASE_MEMORY_DUMP 'v' is missing https://codereview.chromium.org/988893003/diff/200001/src/tracing/trace-event... src/tracing/trace-event.h:916: There are a few flags missing from here _SCOPE_EXTRA, _EXPLICIT_TIMESTAMP, _ASYNC_TTS https://codereview.chromium.org/988893003/diff/200001/src/tracing/trace-event... src/tracing/trace-event.h:917: #define TRACE_EVENT_FLAG_SCOPE_MA \ Seem to have lost the SK on the end? SCOPE_MASK https://codereview.chromium.org/988893003/diff/200001/src/tracing/trace-event... src/tracing/trace-event.h:919: (TRACE_EVENT_FLAG_SCOPE_OFFSET << 1))) This is SCOPE_EXTRA in the base version. https://codereview.chromium.org/988893003/diff/200001/src/tracing/trace-event... src/tracing/trace-event.h:932: // TRACE_EVENT_FLAG_SCOPE_MA. SK
yurys@chromium.org changed reviewers: + caseq@chromium.org
+caseq
+1 to getting this done (and willing to help if needed, as I had a somewhat similar CL before yurys@ kindly pointed me here). We would like to start relying on trace events emitted by v8 in DevTools' Timeline (in particular, crbug.com/508005 depends on this), but would like a finer break-down by categories, as opposed to everything being traced as "v8" now. Being able to pass additional arguments would eventually be useful, too. This all calls for exposing the reach interface to tracing, in particular for categories -- just passing categories via event logger would greatly increase the overhead, as we won't be able to cache pointers to enable flags on the trace site as trace macros do now. Yang and Jochen, would keeping existing TimerEventScope<TimerEventFoo> along with per-isolate logging to file while deprecating SetEventLogger() from public Isolate API address your concern of per-isolate logging? We could also expose isolate id via tracing events as Nat suggests, though I think existing event logger callback do not actually require this.
On 2015/07/08 09:53:04, caseq wrote: > +1 to getting this done (and willing to help if needed, as I had a somewhat > similar CL before yurys@ kindly pointed me here). > > We would like to start relying on trace events emitted by v8 in DevTools' > Timeline (in particular, crbug.com/508005 depends on this), but would like a > finer break-down by categories, as opposed to everything being traced as "v8" > now. Being able to pass additional arguments would eventually be useful, too. > This all calls for exposing the reach interface to tracing, in particular for > categories -- just passing categories via event logger would greatly increase > the overhead, as we won't be able to cache pointers to enable flags on the trace > site as trace macros do now. > > Yang and Jochen, would keeping existing TimerEventScope<TimerEventFoo> along > with per-isolate logging to file while deprecating SetEventLogger() from public > Isolate API address your concern of per-isolate logging? We could also expose > isolate id via tracing events as Nat suggests, though I think existing event > logger callback do not actually require this. I am currently working on adding isolate id (context id) to trace events. How about we land this CL as is, leave the current logging technique for V8 until we get isolate id in trace events? This would allow us to get the V8 team to start using the trace macros right away. Adding the isolate id only happens in tracing code.
On 2015/07/08 16:44:25, fmeawad wrote: > On 2015/07/08 09:53:04, caseq wrote: > > +1 to getting this done (and willing to help if needed, as I had a somewhat > > similar CL before yurys@ kindly pointed me here). > > > > We would like to start relying on trace events emitted by v8 in DevTools' > > Timeline (in particular, crbug.com/508005 depends on this), but would like a > > finer break-down by categories, as opposed to everything being traced as "v8" > > now. Being able to pass additional arguments would eventually be useful, too. > > This all calls for exposing the reach interface to tracing, in particular for > > categories -- just passing categories via event logger would greatly increase > > the overhead, as we won't be able to cache pointers to enable flags on the > trace > > site as trace macros do now. > > > > Yang and Jochen, would keeping existing TimerEventScope<TimerEventFoo> along > > with per-isolate logging to file while deprecating SetEventLogger() from > public > > Isolate API address your concern of per-isolate logging? We could also expose > > isolate id via tracing events as Nat suggests, though I think existing event > > logger callback do not actually require this. > > I am currently working on adding isolate id (context id) to trace events. How > about we land this CL as is, leave the current logging technique for V8 until we > get isolate id in trace events? This would allow us to get the V8 team to start > using the trace macros right away. Adding the isolate id only happens in tracing > code. The context_id stuff landed right? So, does that mean we can move this forward again?
On 2015/08/05 19:01:26, dsinclair wrote: > On 2015/07/08 16:44:25, fmeawad wrote: > > On 2015/07/08 09:53:04, caseq wrote: > > > +1 to getting this done (and willing to help if needed, as I had a somewhat > > > similar CL before yurys@ kindly pointed me here). > > > > > > We would like to start relying on trace events emitted by v8 in DevTools' > > > Timeline (in particular, crbug.com/508005 depends on this), but would like a > > > finer break-down by categories, as opposed to everything being traced as > "v8" > > > now. Being able to pass additional arguments would eventually be useful, > too. > > > This all calls for exposing the reach interface to tracing, in particular > for > > > categories -- just passing categories via event logger would greatly > increase > > > the overhead, as we won't be able to cache pointers to enable flags on the > > trace > > > site as trace macros do now. > > > > > > Yang and Jochen, would keeping existing TimerEventScope<TimerEventFoo> along > > > with per-isolate logging to file while deprecating SetEventLogger() from > > public > > > Isolate API address your concern of per-isolate logging? We could also > expose > > > isolate id via tracing events as Nat suggests, though I think existing event > > > logger callback do not actually require this. > > > > I am currently working on adding isolate id (context id) to trace events. How > > about we land this CL as is, leave the current logging technique for V8 until > we > > get isolate id in trace events? This would allow us to get the V8 team to > start > > using the trace macros right away. Adding the isolate id only happens in > tracing > > code. > > The context_id stuff landed right? So, does that mean we can move this forward > again? Yes, the chrome changes landed. I will rework this CL to include the changes.
dsinclair, PTAL.
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988893003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/988893003/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm w/ nits https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h File include/v8-tracing.h (right): https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h#ne... include/v8-tracing.h:32: // These values must be in sync with macro values in trace_event.h in nit: s/trace_event.h/trace_log.h https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h#ne... include/v8-tracing.h:41: }; Should we add a placeholder for ETW_EXPORT 1 << 3 here? Or comment that it's used? https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h#ne... include/v8-tracing.h:59: }; DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h#ne... include/v8-tracing.h:60: } // namespace v8 nit: blank line above https://codereview.chromium.org/988893003/diff/240001/src/tracing/event-trace... File src/tracing/event-tracer.cc (right): https://codereview.chromium.org/988893003/diff/240001/src/tracing/event-trace... src/tracing/event-tracer.cc:44: } nit: no {}'s on single line bodies https://codereview.chromium.org/988893003/diff/240001/src/tracing/trace-event.h File src/tracing/trace-event.h (right): https://codereview.chromium.org/988893003/diff/240001/src/tracing/trace-event... src/tracing/trace-event.h:60: Should this be UNIMPLEMENTED()? https://codereview.chromium.org/988893003/diff/240001/src/tracing/trace-event... src/tracing/trace-event.h:111: // FIXME crbug?
Addressed Dan's comments. Yang, PTAL! https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h File include/v8-tracing.h (right): https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h#ne... include/v8-tracing.h:32: // These values must be in sync with macro values in trace_event.h in On 2015/08/25 13:56:22, dsinclair wrote: > nit: s/trace_event.h/trace_log.h Done. https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h#ne... include/v8-tracing.h:41: }; On 2015/08/25 13:56:22, dsinclair wrote: > Should we add a placeholder for ETW_EXPORT 1 << 3 here? Or comment that it's > used? I don't think there is a problem of just adding it? https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h#ne... include/v8-tracing.h:59: }; On 2015/08/25 13:56:22, dsinclair wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/988893003/diff/240001/include/v8-tracing.h#ne... include/v8-tracing.h:60: } // namespace v8 On 2015/08/25 13:56:22, dsinclair wrote: > nit: blank line above Done. https://codereview.chromium.org/988893003/diff/240001/src/tracing/event-trace... File src/tracing/event-tracer.cc (right): https://codereview.chromium.org/988893003/diff/240001/src/tracing/event-trace... src/tracing/event-tracer.cc:44: } On 2015/08/25 13:56:22, dsinclair wrote: > nit: no {}'s on single line bodies Done. https://codereview.chromium.org/988893003/diff/240001/src/tracing/trace-event.h File src/tracing/trace-event.h (right): https://codereview.chromium.org/988893003/diff/240001/src/tracing/trace-event... src/tracing/trace-event.h:60: On 2015/08/25 13:56:22, dsinclair wrote: > Should this be UNIMPLEMENTED()? No, because it is OK for it to have no implementation, as it gets called from all trace events. For those trace events, they will work as expected except without memory tracing. Added a comment. https://codereview.chromium.org/988893003/diff/240001/src/tracing/trace-event... src/tracing/trace-event.h:111: // FIXME On 2015/08/25 13:56:22, dsinclair wrote: > crbug? I fixed the implementation, was a left out of a copy from Skia that has long been fix, thanks for the catch.
https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h File include/v8-tracing.h (right): https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h#ne... include/v8-tracing.h:49: virtual EventTracer::Handle AddTraceEvent( I'm very positive about the CL in general, but rather than introducing additional class for embedders to implement, can we just add these to v8::Platform? Besides simplifying the API slightly, there's another small advantage in that docs already stress well the need for InitializePlatform to be called before v8 initialization, which I think is critical in case of tracing (due to caching of category pointers).
On 2015/08/25 22:02:46, caseq wrote: > https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h > File include/v8-tracing.h (right): > > https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h#ne... > include/v8-tracing.h:49: virtual EventTracer::Handle AddTraceEvent( > I'm very positive about the CL in general, but rather than introducing > additional class for embedders to implement, can we just add these to > v8::Platform? Besides simplifying the API slightly, there's another small > advantage in that docs already stress well the need for InitializePlatform to be > called before v8 initialization, which I think is critical in case of tracing > (due to caching of category pointers). I still feel very uncomfortable about dropping a copy of a huge file from Chromium (trace-event-common.h) into V8 without any automated way to know whether it needs to be updated. We have no precedence for this sort of thing. I would be a lot more comfortable if we had some way, maybe via gclient, to synchronize.
On 2015/08/27 10:48:56, Yang wrote: > On 2015/08/25 22:02:46, caseq wrote: > > https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h > > File include/v8-tracing.h (right): > > > > > https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h#ne... > > include/v8-tracing.h:49: virtual EventTracer::Handle AddTraceEvent( > > I'm very positive about the CL in general, but rather than introducing > > additional class for embedders to implement, can we just add these to > > v8::Platform? Besides simplifying the API slightly, there's another small > > advantage in that docs already stress well the need for InitializePlatform to > be > > called before v8 initialization, which I think is critical in case of tracing > > (due to caching of category pointers). > > I still feel very uncomfortable about dropping a copy of a huge file from > Chromium (trace-event-common.h) into V8 without any automated way to know > whether it needs to be updated. We have no precedence for this sort of thing. I > would be a lot more comfortable if we had some way, maybe via gclient, to > synchronize. The common.h file is there to be copied into other projects. We add to the file but we don't modify the current values of anything. So, while chrome may get a new macro that v8 doesn't have, until v8 needs that macro the header can be out of sync. This is what is currently being done in blink, skia and nacl. If v8 wants to add new macros they should be added into the chrome version first, as mentioned in the .h file.
On 2015/08/27 13:05:52, dsinclair wrote: > On 2015/08/27 10:48:56, Yang wrote: > > On 2015/08/25 22:02:46, caseq wrote: > > > https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h > > > File include/v8-tracing.h (right): > > > > > > > > > https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h#ne... > > > include/v8-tracing.h:49: virtual EventTracer::Handle AddTraceEvent( > > > I'm very positive about the CL in general, but rather than introducing > > > additional class for embedders to implement, can we just add these to > > > v8::Platform? Besides simplifying the API slightly, there's another small > > > advantage in that docs already stress well the need for InitializePlatform > to > > be > > > called before v8 initialization, which I think is critical in case of > tracing > > > (due to caching of category pointers). > > > > I still feel very uncomfortable about dropping a copy of a huge file from > > Chromium (trace-event-common.h) into V8 without any automated way to know > > whether it needs to be updated. We have no precedence for this sort of thing. > I > > would be a lot more comfortable if we had some way, maybe via gclient, to > > synchronize. > > > The common.h file is there to be copied into other projects. We add to the file > but we don't modify the current values of anything. So, while chrome may get a > new macro that v8 doesn't have, until v8 needs that macro the header can be out > of sync. This is what is currently being done in blink, skia and nacl. > > If v8 wants to add new macros they should be added into the chrome version > first, as mentioned in the .h file. Yang, ping.
On 2015/08/31 20:27:43, fmeawad wrote: > > Yang, ping. From the display name, it looks like Yang is OOO until mid-October. We'd like to get this landed a bit sooner then that, so is there someone else from the v8 team who can take a look?
i'm back & looking. it would be nice if we could move those files to a separate directory in chrome, so other projects like v8 can pull it in via DEPS (we can export individual directories from chrome as separate git repositories). I agree that manually syncing a large file across multiple repos isn't a good thing[tm] was moving this file to e.g. base/tracing/exported/ or similar possible?
On 2015/09/07 14:24:13, jochen wrote: > i'm back & looking. > > it would be nice if we could move those files to a separate directory in chrome, > so other projects like v8 can pull it in via DEPS (we can export individual > directories from chrome as separate git repositories). > > I agree that manually syncing a large file across multiple repos isn't a good > thing[tm] > > was moving this file to e.g. base/tracing/exported/ or similar possible? We agree that the copying isn't a good solution, but it's a larger fix to change to having a sync'd directory. This file is actually the first step in getting us to a better place as this file requires no modification by the copiers. Prior to having this we'd have to modify the header for each client to work with their infrastructure. So, we'd like to get there, we aren't there yet and we don't have the resources to get us there in the short term. We'd like to get v8 tracing working in the short term if at all possible.
not sure why this would be a larger fix? it would require moving the files in chromium (large CL but automated), filing an infra ticket, and replacing the files in the CLs with a DEPS entry. some comments inline. what's the plan with all the new trace macros? will they supersede the LOG() macros currently used in v8? if yes, who's going to do that? https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h File include/v8-tracing.h (right): https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h#ne... include/v8-tracing.h:49: virtual EventTracer::Handle AddTraceEvent( agreed. Either it should be part of the platform, or passed via the Isolate::CreateParams. we shouldn't have a global static event tracer, but it should be scoped to an isolate (eventually, the v8::Platform will also be passed to Isolate::New) https://codereview.chromium.org/988893003/diff/260001/src/tracing/trace-event.h File src/tracing/trace-event.h (right): https://codereview.chromium.org/988893003/diff/260001/src/tracing/trace-event... src/tracing/trace-event.h:238: (uint64_t) v8::internal::Isolate::UnsafeCurrent don't use this method. Isolate* should be always passed around explicitly, you can't rely on getting the Isolate via any of the *Current() methods
https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h File include/v8-tracing.h (right): https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h#ne... include/v8-tracing.h:49: virtual EventTracer::Handle AddTraceEvent( On 2015/09/08 14:20:34, jochen wrote: > we shouldn't have a global static event tracer, but it should be scoped to an > isolate (eventually, the v8::Platform will also be passed to Isolate::New) Current trace macros implementation relies on static variables for caching pointers to category enable flags (see https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...) -- so I'm afraid that allowing setting tracers per isolate may create a false impression that one could use different tracer implementation for different isolates, while we actually rely on tracer being a singleton.
On 2015/09/08 at 17:57:49, caseq wrote: > https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h > File include/v8-tracing.h (right): > > https://codereview.chromium.org/988893003/diff/260001/include/v8-tracing.h#ne... > include/v8-tracing.h:49: virtual EventTracer::Handle AddTraceEvent( > On 2015/09/08 14:20:34, jochen wrote: > > we shouldn't have a global static event tracer, but it should be scoped to an > > isolate (eventually, the v8::Platform will also be passed to Isolate::New) > > Current trace macros implementation relies on static variables for caching pointers to category enable flags (see https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t...) -- so I'm afraid that allowing setting tracers per isolate may create a false impression that one could use different tracer implementation for different isolates, while we actually rely on tracer being a singleton. I'm not saying that chromium's tracing shouldn't use static variables, but v8 should definitely not add new ones.
rmcilroy@chromium.org changed reviewers: + rmcilroy@chromium.org
info@bnoordhuis.nl changed reviewers: + info@bnoordhuis.nl
https://codereview.chromium.org/988893003/diff/280001/include/v8-platform.h File include/v8-platform.h (right): https://codereview.chromium.org/988893003/diff/280001/include/v8-platform.h#n... include/v8-platform.h:126: const char* name, uint64_t handle) = 0; These really need elucidating comments, either inline or as links to online or in-repo documentation. Style issue: I think V8 prefers snake_case over camelCase, i.e., category_enabled_flag instead of categoryEnabledFlag. https://codereview.chromium.org/988893003/diff/280001/src/libplatform/default... File src/libplatform/default-platform.cc (right): https://codereview.chromium.org/988893003/diff/280001/src/libplatform/default... src/libplatform/default-platform.cc:190: static const char* dummy = "dummy"; `static const char[] dummy = ...`? https://codereview.chromium.org/988893003/diff/280001/src/log-inl.h File src/log-inl.h (right): https://codereview.chromium.org/988893003/diff/280001/src/log-inl.h#newcode42 src/log-inl.h:42: TRACE_EVENT_END_WITH_CONTEXT_ID0("v8", name, (uint64_t)isolate); Style: use static_cast/reinterpret_cast here. Should this block be hoisted to one of the outer if statements? It currently isn't reached when logging is disabled. https://codereview.chromium.org/988893003/diff/280001/test/cctest/test-trace-... File test/cctest/test-trace-event.cc (right): https://codereview.chromium.org/988893003/diff/280001/test/cctest/test-trace-... test/cctest/test-trace-event.cc:29: int numArgs; Style nit: mildly inconsistent mix of camelCase and snake_case.
Description was changed from ========== Implement tracing interface for v8 This is based on the Skia Implementation. More on the project can be found here: https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4... The V8 Tracing platform will replace the isolate->event_logger(). But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set. Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing. Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation) BUG=446513 LOG=N ========== to ========== Implement tracing interface for v8 This is based on the Skia Implementation. More on the project can be found here: https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4... The V8 Tracing platform will replace the isolate->event_logger(). But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set. Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing. Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation) BUG=4558 LOG=N ==========
Description was changed from ========== Implement tracing interface for v8 This is based on the Skia Implementation. More on the project can be found here: https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4... The V8 Tracing platform will replace the isolate->event_logger(). But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set. Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing. Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation) BUG=4558 LOG=N ========== to ========== Implement tracing interface for v8 This is based on the Skia Implementation. More on the project can be found here: https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4... The V8 Tracing platform will replace the isolate->event_logger(). But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set. Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing. Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation) BUG=4560 LOG=N ==========
Description was changed from ========== Implement tracing interface for v8 This is based on the Skia Implementation. More on the project can be found here: https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4... The V8 Tracing platform will replace the isolate->event_logger(). But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set. Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing. Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation) BUG=4560 LOG=N ========== to ========== Implement tracing interface for v8 This is based on the Skia Implementation. More on the project can be found here: https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4... The V8 Tracing platform will replace the isolate->event_logger(). But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set. Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing. Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation) BUG=v8:4560 LOG=N ==========
I have addressed all comments, Major changes include: 1- removing trace-event-common.h and put it in DEPS instead 2- removing "src/v8.h" from trace-event.h, and add trace-event.cc to be used to GetCurrentPlatform 3- Filing a bug for the local static variable and including it in the code as a TODO 4- Address styling comments. https://codereview.chromium.org/988893003/diff/280001/include/v8-platform.h File include/v8-platform.h (right): https://codereview.chromium.org/988893003/diff/280001/include/v8-platform.h#n... include/v8-platform.h:126: const char* name, uint64_t handle) = 0; On 2015/10/07 at 18:31:54, noordhuis wrote: > These really need elucidating comments, either inline or as links to online or in-repo documentation. > > Style issue: I think V8 prefers snake_case over camelCase, i.e., category_enabled_flag instead of categoryEnabledFlag. Done https://codereview.chromium.org/988893003/diff/280001/src/libplatform/default... File src/libplatform/default-platform.cc (right): https://codereview.chromium.org/988893003/diff/280001/src/libplatform/default... src/libplatform/default-platform.cc:190: static const char* dummy = "dummy"; On 2015/10/07 18:31:54, noordhuis wrote: > `static const char[] dummy = ...`? Done. https://codereview.chromium.org/988893003/diff/280001/src/log-inl.h File src/log-inl.h (right): https://codereview.chromium.org/988893003/diff/280001/src/log-inl.h#newcode42 src/log-inl.h:42: TRACE_EVENT_END_WITH_CONTEXT_ID0("v8", name, (uint64_t)isolate); On 2015/10/07 18:31:54, noordhuis wrote: > Style: use static_cast/reinterpret_cast here. > > Should this block be hoisted to one of the outer if statements? It currently > isn't reached when logging is disabled. Done. https://codereview.chromium.org/988893003/diff/280001/test/cctest/test-trace-... File test/cctest/test-trace-event.cc (right): https://codereview.chromium.org/988893003/diff/280001/test/cctest/test-trace-... test/cctest/test-trace-event.cc:29: int numArgs; On 2015/10/07 18:31:54, noordhuis wrote: > Style nit: mildly inconsistent mix of camelCase and snake_case. Done.
LGTM with comments. I think I'm missing a test case where we can distinguish trace events by isolate (using the isolate as id?). https://codereview.chromium.org/988893003/diff/320001/include/v8-platform.h File include/v8-platform.h (right): https://codereview.chromium.org/988893003/diff/320001/include/v8-platform.h#n... include/v8-platform.h:125: * Add a trace event to the platform tracing system. Can you add a comment about what this method returns? It seems unclear to me. https://codereview.chromium.org/988893003/diff/320001/src/tracing/trace-event.h File src/tracing/trace-event.h (right): https://codereview.chromium.org/988893003/diff/320001/src/tracing/trace-event... src/tracing/trace-event.h:276: class TraceID { Is this ID used to represent the isolate? If yes, then adding all the different flavors of DontMangle and ForceMangle seems a bit overkill if we are only going to use DontMangle(const void*).
> I think I'm missing a test case where we can distinguish trace events by isolate (using the isolate as id?). I forgot to mention, that part of the major changes in the last patch is to get rid of context_id in favor of the much more general and ExecutionContext system we are adding to tracing. I filed https://code.google.com/p/v8/issues/detail?id=4565 to track landing ExecutionContext in v8, and blocked replacing LOG on that one.
https://codereview.chromium.org/988893003/diff/320001/include/v8-platform.h File include/v8-platform.h (right): https://codereview.chromium.org/988893003/diff/320001/include/v8-platform.h#n... include/v8-platform.h:125: * Add a trace event to the platform tracing system. On 2015/11/19 at 09:30:32, Yang wrote: > Can you add a comment about what this method returns? It seems unclear to me. I have updated the description on all the methods. https://codereview.chromium.org/988893003/diff/320001/src/tracing/trace-event.h File src/tracing/trace-event.h (right): https://codereview.chromium.org/988893003/diff/320001/src/tracing/trace-event... src/tracing/trace-event.h:276: class TraceID { On 2015/11/19 at 09:30:32, Yang wrote: > Is this ID used to represent the isolate? If yes, then adding all the different flavors of DontMangle and ForceMangle seems a bit overkill if we are only going to use DontMangle(const void*). DontMangle is used by the TRACE_EVENT_OBJECT_* macros which can be used to "track the life time and value of arbitrary client objects." therefore we need the full list here.
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988893003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/988893003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_nodcheck_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_nodcheck_rel/build...)
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988893003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/988893003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988893003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/988893003/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
The CQ bit was checked by fmeawad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dsinclair@chromium.org, yangguo@chromium.org Link to the patchset: https://codereview.chromium.org/988893003/#ps400001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988893003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/988893003/400001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
i guess v8 needs to depend on a target defined in the new DEPS entry to build with chromium https://codereview.chromium.org/988893003/diff/400001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/988893003/diff/400001/BUILD.gn#newcode1271 BUILD.gn:1271: "src/tracing/trace-event.h", the cc file needs to be added here as well
I have also landed https://codereview.chromium.org/1469303004/ yesterday to get the common DEPS to roll into chromium, waiting for a v8-roll. https://codereview.chromium.org/988893003/diff/400001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/988893003/diff/400001/BUILD.gn#newcode1271 BUILD.gn:1271: "src/tracing/trace-event.h", On 2015/11/26 13:28:35, jochen wrote: > the cc file needs to be added here as well Done.
https://codereview.chromium.org/988893003/diff/420001/DEPS File DEPS (right): https://codereview.chromium.org/988893003/diff/420001/DEPS#newcode16 DEPS:16: "v8/src/tracing/common": I think this line should be v8/base/trace_event/common
On 2015/12/01 at 17:04:41, jochen wrote: > https://codereview.chromium.org/988893003/diff/420001/DEPS > File DEPS (right): > > https://codereview.chromium.org/988893003/diff/420001/DEPS#newcode16 > DEPS:16: "v8/src/tracing/common": > I think this line should be v8/base/trace_event/common I'm taking care of this..
https://codereview.chromium.org/988893003/diff/420001/include/v8-platform.h File include/v8-platform.h (right): https://codereview.chromium.org/988893003/diff/420001/include/v8-platform.h#n... include/v8-platform.h:120: virtual const uint8_t* GetCategoryGroupEnabled(const char* name) = 0; we might also want to provide default implementations here to make it easier for embedders to roll forward
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988893003/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/988893003/460001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_chromium_gn_rel on tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_chromium_gn_rel/bu...)
The CQ bit was checked by fmeawad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988893003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/988893003/480001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fmeawad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yangguo@chromium.org, dsinclair@chromium.org Link to the patchset: https://codereview.chromium.org/988893003/#ps480001 (title: "Update BUILD.gn to include trace_event_common.h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988893003/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/988893003/480001
Message was sent while issue was closed.
Committed patchset #25 (id:480001)
Message was sent while issue was closed.
Description was changed from ========== Implement tracing interface for v8 This is based on the Skia Implementation. More on the project can be found here: https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4... The V8 Tracing platform will replace the isolate->event_logger(). But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set. Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing. Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation) BUG=v8:4560 LOG=N ========== to ========== Implement tracing interface for v8 This is based on the Skia Implementation. More on the project can be found here: https://docs.google.com/a/chromium.org/document/d/1_4LAnInOB8tM_DLjptWiszRwa4... The V8 Tracing platform will replace the isolate->event_logger(). But since the current embedders (namely chromium) currently use the isolate->event_logger, I made the default implementation (event-tracer) call into isolate->event_logger if an event_logger was set. Once the embedders properly implement the interface (for example in chromium it would look like this: https://codereview.chromium.org/707273005/), the default implementation will be doing nothing. Once the embedders side is fixed, we will change how V8 uses the tracing framework beyond the call from Logger:CallEventLogger. (which would also include a d8 implementation) BUG=v8:4560 LOG=N Committed: https://crrev.com/70a7c754bf3445a8b783b75ca2a7aa34cdeb080b Cr-Commit-Position: refs/heads/master@{#32959} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/70a7c754bf3445a8b783b75ca2a7aa34cdeb080b Cr-Commit-Position: refs/heads/master@{#32959}
Message was sent while issue was closed.
On 2015/12/17 at 18:48:42, commit-bot wrote: > Patchset 25 (id:??) landed as https://crrev.com/70a7c754bf3445a8b783b75ca2a7aa34cdeb080b > Cr-Commit-Position: refs/heads/master@{#32959} This breaks my interactive builds of V8.
Message was sent while issue was closed.
On 2015/12/17 at 21:30:35, Dan Ehrenberg wrote: > On 2015/12/17 at 18:48:42, commit-bot wrote: > > Patchset 25 (id:??) landed as https://crrev.com/70a7c754bf3445a8b783b75ca2a7aa34cdeb080b > > Cr-Commit-Position: refs/heads/master@{#32959} > > This breaks my interactive builds of V8. Oh sorry, this was from a missing 'gclient sync'. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
