Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
792903003:
V8 Sampling Profiler: Collect V8 JitCodeEvents in tracing (Closed)
|
Created:
5 years, 11 months ago by alph Modified:
5 years, 10 months ago CC:
aandrey+blink_chromium.org, chromium-reviews, darin-cc_chromium.org, devtools-reviews_chromium.org, dsinclair+watch_chromium.org, erikwright+watch_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, paulirish+reviews_chromium.org, pfeldman, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionV8 Sampling Profiler: Collect V8 JitCodeEvents in tracing
BUG=406277
Committed: https://crrev.com/c8094f8f44f4ad509c3fe003dfcee210beb77216
Cr-Commit-Position: refs/heads/master@{#318025}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Addressing comments #Patch Set 3 : Do not enable profiling in continuous record mode. #Patch Set 4 : Rebaseline. #
Total comments: 6
Patch Set 5 : Addressing comments. #Patch Set 6 : Fix quotes. #Patch Set 7 : Rename category v8_cpu_profile -> v8.cpu_profile #Patch Set 8 : Get rid of CurrentThreadHandle #Patch Set 9 : Fix the test under Debug mode. #
Messages
Total messages: 48 (11 generated)
alph@chromium.org changed reviewers: + dsinclair@chromium.org, fmeawad@chromium.org, jochen@chromium.org, yurys@chromium.org
Please take a look. Thanks!
lgtm
yurys@chromium.org changed reviewers: + nduca@chromium.org
https://codereview.chromium.org/792903003/diff/1/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/792903003/diff/1/base/debug/trace_event.h#new... base/debug/trace_event.h:355: TRACE_EVENT_FLAG_NONE | TRACE_EVENT_SCOPE_THREAD) TRACE_EVENT_SCOPE_THREAD here and below? https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:37: void Start(); Please add a comment that these methods can be called on different thread. https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:46: JitCodeEventToTraceFormat(const v8::JitCodeEvent* event); Wrong indentation. https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:81: TRACE_EVENT_SAMPLE_METADATA1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), How does this relate to the stack trace format described in [1]? Should we reconsider the format for the JS samples and reflect it in that document? Personally I like the approach when we write code events and stack samples as instant events and do all the post-processing later on the front-end. This would allow to drastically simplify the back-end as we wouldn't need to keep the code tree there and could probably get rid of the additional processing thread. On the other hand that would require two duplicate implementations of the processor - one in DevTools front-end and another in the trace viewer. WDYT? In any case the document [1] should describe actual format we want to implement. Also it is not quite clear how these events correspond to the first step mentioned in the bug when we store whole profile in a single instant event. [1] https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:87: TRACE_EVENT_SAMPLE_METADATA1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), Why is this metadata event rather than instant one? https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:112: data->SetString("code_start", PtrToString(event->code_start)); Do we want to see isolate identifier as one of the properties? https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:146: RenderThreadSampler() Why do we need a subclass here? Wouldn't Sampler::createForCurrentThread() work ? https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:182: : render_thread_sampler_(render_thread_sampler), May be pass an array of all samplers? https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:204: for (auto sampler : samplers_) { auto -> Sampler* here and below
dsinclair@chromium.org changed reviewers: + skyostil@chromium.org
+skyostil who, I believe, added the stack frame code.
https://codereview.chromium.org/792903003/diff/1/base/debug/trace_event.h File base/debug/trace_event.h (right): https://codereview.chromium.org/792903003/diff/1/base/debug/trace_event.h#new... base/debug/trace_event.h:355: TRACE_EVENT_FLAG_NONE | TRACE_EVENT_SCOPE_THREAD) On 2015/01/14 13:41:36, yurys wrote: > TRACE_EVENT_SCOPE_THREAD here and below? The flag and scope are different domains, so I specified both. Actually I copypasted that from INSTANT_EVENT. Anyway these are gone. https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:37: void Start(); On 2015/01/14 13:41:36, yurys wrote: > Please add a comment that these methods can be called on different thread. Done. https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:46: JitCodeEventToTraceFormat(const v8::JitCodeEvent* event); On 2015/01/14 13:41:37, yurys wrote: > Wrong indentation. That's not me! That's git cl format https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:81: TRACE_EVENT_SAMPLE_METADATA1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), On 2015/01/14 13:41:36, yurys wrote: > How does this relate to the stack trace format described in [1]? Should we > reconsider the format for the JS samples and reflect it in that document? Yes, it's a different format. I'm not feeling comfortable to break into the document. We could wait for Nat commenting on the topic. The good point about the format introduced with this patch is that there's no in fact a new format. Everything is described in the trace events sequence terms. Just as close to the probed data as possible. The client may then do any postprocessing it need, e.g. aggregate data, coalesce neighbor samples, etc. > Personally I like the approach when we write code events and stack samples as > instant events and do all the post-processing later on the front-end. This would > allow to drastically simplify the back-end as we wouldn't need to keep the code > tree there and could probably get rid of the additional processing thread. On > the other hand that would require two duplicate implementations of the processor > - one in DevTools front-end and another in the trace viewer. WDYT? In any case > the document [1] should describe actual format we want to implement. I see the following pros/cons in the [1]: + it requires small amount of processing on the client side. - the format introduces new entities instead of plain array of trace events. - it aggregates data on samples, so it becomes impossible to draw profile in a timeline (flamechart) way. - it requires backend side post-processing. > Also it is not quite clear how these events correspond to the first step > mentioned in the bug when we store whole profile in a single instant event. We decided not to go this way. > > [1] > https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy... https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:87: TRACE_EVENT_SAMPLE_METADATA1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), On 2015/01/14 13:41:36, yurys wrote: > Why is this metadata event rather than instant one? I tried not to garble the trace view with the samples. But yes, if one has enabled the sampling category let him see the samples and jit code events. Turned them back into instant events. https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:112: data->SetString("code_start", PtrToString(event->code_start)); On 2015/01/14 13:41:36, yurys wrote: > Do we want to see isolate identifier as one of the properties? It is not necessary. The code_start uniquely identifies code objects. We may add Isolate later if needed, but I assume it will just be the same for all events on the thread. https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:146: RenderThreadSampler() On 2015/01/14 13:41:36, yurys wrote: > Why do we need a subclass here? Wouldn't Sampler::createForCurrentThread() work > ? Done. https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:182: : render_thread_sampler_(render_thread_sampler), On 2015/01/14 13:41:37, yurys wrote: > May be pass an array of all samplers? I'm not yet sure who will be responsible for creating samplers for workers. Let me update on that later. https://codereview.chromium.org/792903003/diff/1/content/renderer/devtools/v8... content/renderer/devtools/v8_sampling_profiler.cc:204: for (auto sampler : samplers_) { On 2015/01/14 13:41:36, yurys wrote: > auto -> Sampler* here and below Done.
skyostil@chromium.org changed reviewers: + vmiura@chromium.org
On 2015/01/14 14:22:00, dsinclair wrote: > +skyostil who, I believe, added the stack frame code. Actually I just stole vmiura's code, but I can take a look regardless.
On 2015/01/14 15:14:10, Sami wrote: > On 2015/01/14 14:22:00, dsinclair wrote: > > +skyostil who, I believe, added the stack frame code. > > Actually I just stole vmiura's code, but I can take a look regardless. Sorry, what is stack frame code?
On 2015/01/14 at 15:15:33, alph wrote: > On 2015/01/14 15:14:10, Sami wrote: > > On 2015/01/14 14:22:00, dsinclair wrote: > > > +skyostil who, I believe, added the stack frame code. > > > > Actually I just stole vmiura's code, but I can take a look regardless. > > Sorry, what is stack frame code? The part of the tracing format yurys@ was referring too in comment #5.
On 2015/01/14 15:15:33, alph wrote: > On 2015/01/14 15:14:10, Sami wrote: > > On 2015/01/14 14:22:00, dsinclair wrote: > > > +skyostil who, I believe, added the stack frame code. > > > > Actually I just stole vmiura's code, but I can take a look regardless. > > Sorry, what is stack frame code? There's code in trace viewer that knows how to visualize sample call stacks from the perf profiler. This relies on having a tree of stack frames in the trace as described in [1]. It doesn't look like you're saving stack frames in this patch so maybe this is a little premature, but once that becomes interesting we should figure out if the same data structure is fit for v8 samples too or whether we need something better.
That said, it would be nice if the format of the sample itself would be the same
even if there's no stack frame (beyond the code name that you've got here?).
For reference, here's what a perf sample contains:
{
'cpu': 0, 'tid': 1, 'ts': 1000.0,
'name': 'cycles:HG', 'sf': 3, 'weight': 1
}
Do you think these jit events would fit into that model?
On 2015/01/14 15:32:45, Sami wrote:
> That said, it would be nice if the format of the sample itself would be the
same
> even if there's no stack frame (beyond the code name that you've got here?).
>
> For reference, here's what a perf sample contains:
>
> {
> 'cpu': 0, 'tid': 1, 'ts': 1000.0,
> 'name': 'cycles:HG', 'sf': 3, 'weight': 1
> }
>
> Do you think these jit events would fit into that model?
Ok, let me describe in short the current schema of v8 sampling profiler we're
trying to move into chromium.
There's a sampler thread that once in a millisecond stops the v8 thread and
invokes a V8 API for a call stack. The callstack is plain array of PC addresses
of frames.
As long as the V8 heap is dynamic, i.e. functions may move around the heap
during execution (well, during GC) there are JIT code events. They are used to
map addresses to function names. JitCodeAdded is fired when a new code object is
created. It has the info about function name, start address and length.
JitCodeMoved is invoked when GC moves code object from one address to another,
And JitCodeDeleted is called when the code is deleted.
Current v8 profiler logs samples along with Jit code events in a buffer without
any processing for the sake of performance, just to minimize the profile
skewing.
That is what I wanted to move to the tracing. I.e. the plan is to record samples
and Jit code events as instant trace events and let the client to do the
postprocessing.
If we want to store the profile data in [1] format, we need to map frame
addresses to functions on the fly. Or alternatively store all the information
aside of tracing buffers, then postprocess it when tracing is stopped, and
finally inject samples and frames into the TraceLog. Once the post processing is
done, Jit code events are not needed anymore.
On 2015/01/14 15:49:09, alph wrote:
> On 2015/01/14 15:32:45, Sami wrote:
> > That said, it would be nice if the format of the sample itself would be the
> same
> > even if there's no stack frame (beyond the code name that you've got here?).
> >
> > For reference, here's what a perf sample contains:
> >
> > {
> > 'cpu': 0, 'tid': 1, 'ts': 1000.0,
> > 'name': 'cycles:HG', 'sf': 3, 'weight': 1
> > }
> >
> > Do you think these jit events would fit into that model?
>
> Ok, let me describe in short the current schema of v8 sampling profiler we're
> trying to move into chromium.
>
> There's a sampler thread that once in a millisecond stops the v8 thread and
> invokes a V8 API for a call stack. The callstack is plain array of PC
addresses
> of frames.
> As long as the V8 heap is dynamic, i.e. functions may move around the heap
> during execution (well, during GC) there are JIT code events. They are used to
> map addresses to function names. JitCodeAdded is fired when a new code object
is
> created. It has the info about function name, start address and length.
> JitCodeMoved is invoked when GC moves code object from one address to another,
> And JitCodeDeleted is called when the code is deleted.
>
> Current v8 profiler logs samples along with Jit code events in a buffer
without
> any processing for the sake of performance, just to minimize the profile
> skewing.
> That is what I wanted to move to the tracing. I.e. the plan is to record
samples
> and Jit code events as instant trace events and let the client to do the
> postprocessing.
>
> If we want to store the profile data in [1] format, we need to map frame
> addresses to functions on the fly. Or alternatively store all the information
> aside of tracing buffers, then postprocess it when tracing is stopped, and
> finally inject samples and frames into the TraceLog. Once the post processing
is
> done, Jit code events are not needed anymore.
Thanks for the explanation, that makes things a little clearer. Actually I
should've mentioned that perf tracing is also done as a postprocess because
anything else would similarly have too much overhead. I think it's fine to do
the same thing here and just record the minimum amount of data we need to
reconstruct everything later.
I think the important thing is that in the end the sample data format is
standard so the UI doesn't need to care where it came from. I guess what I'm
unsure about is where the postprocessing step happens? For reference, for perf
it's done by [1] (which isn't very ideal).
[1]
https://code.google.com/p/chromium/codesearch#chromium/src/tools/profile_chro...
On 2015/01/14 16:02:37, Sami wrote:
> On 2015/01/14 15:49:09, alph wrote:
> > On 2015/01/14 15:32:45, Sami wrote:
> > > That said, it would be nice if the format of the sample itself would be
the
> > same
> > > even if there's no stack frame (beyond the code name that you've got
here?).
>
> > >
> > > For reference, here's what a perf sample contains:
> > >
> > > {
> > > 'cpu': 0, 'tid': 1, 'ts': 1000.0,
> > > 'name': 'cycles:HG', 'sf': 3, 'weight': 1
> > > }
> > >
> > > Do you think these jit events would fit into that model?
> >
> > Ok, let me describe in short the current schema of v8 sampling profiler
we're
> > trying to move into chromium.
> >
> > There's a sampler thread that once in a millisecond stops the v8 thread and
> > invokes a V8 API for a call stack. The callstack is plain array of PC
> addresses
> > of frames.
> > As long as the V8 heap is dynamic, i.e. functions may move around the heap
> > during execution (well, during GC) there are JIT code events. They are used
to
> > map addresses to function names. JitCodeAdded is fired when a new code
object
> is
> > created. It has the info about function name, start address and length.
> > JitCodeMoved is invoked when GC moves code object from one address to
another,
> > And JitCodeDeleted is called when the code is deleted.
> >
> > Current v8 profiler logs samples along with Jit code events in a buffer
> without
> > any processing for the sake of performance, just to minimize the profile
> > skewing.
> > That is what I wanted to move to the tracing. I.e. the plan is to record
> samples
> > and Jit code events as instant trace events and let the client to do the
> > postprocessing.
> >
> > If we want to store the profile data in [1] format, we need to map frame
> > addresses to functions on the fly. Or alternatively store all the
information
> > aside of tracing buffers, then postprocess it when tracing is stopped, and
> > finally inject samples and frames into the TraceLog. Once the post
processing
> is
> > done, Jit code events are not needed anymore.
>
> Thanks for the explanation, that makes things a little clearer. Actually I
> should've mentioned that perf tracing is also done as a postprocess because
> anything else would similarly have too much overhead. I think it's fine to do
> the same thing here and just record the minimum amount of data we need to
> reconstruct everything later.
>
> I think the important thing is that in the end the sample data format is
> standard so the UI doesn't need to care where it came from. I guess what I'm
> unsure about is where the postprocessing step happens? For reference, for perf
> it's done by [1] (which isn't very ideal).
>
> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/profile_chro...
AFAICT neither TraceLog nor TracingHostMsg_TraceDataCollected IPC event do
support anything but traceEvents part of the format.
The actual format is written in tracing_controller which also currently knows
nothing about sampling and just writes the traceEvents portion.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...
So it seems to ok to write jit code events and samples as trace events and make
the client, e.g. trace_controller do the post processing. wdyt?
On 2015/01/15 15:43:14, alph wrote:
> On 2015/01/14 16:02:37, Sami wrote:
> > On 2015/01/14 15:49:09, alph wrote:
> > > On 2015/01/14 15:32:45, Sami wrote:
> > > > That said, it would be nice if the format of the sample itself would be
> the
> > > same
> > > > even if there's no stack frame (beyond the code name that you've got
> here?).
> >
> > > >
> > > > For reference, here's what a perf sample contains:
> > > >
> > > > {
> > > > 'cpu': 0, 'tid': 1, 'ts': 1000.0,
> > > > 'name': 'cycles:HG', 'sf': 3, 'weight': 1
> > > > }
> > > >
> > > > Do you think these jit events would fit into that model?
> > >
> > > Ok, let me describe in short the current schema of v8 sampling profiler
> we're
> > > trying to move into chromium.
> > >
> > > There's a sampler thread that once in a millisecond stops the v8 thread
and
> > > invokes a V8 API for a call stack. The callstack is plain array of PC
> > addresses
> > > of frames.
> > > As long as the V8 heap is dynamic, i.e. functions may move around the heap
> > > during execution (well, during GC) there are JIT code events. They are
used
> to
> > > map addresses to function names. JitCodeAdded is fired when a new code
> object
> > is
> > > created. It has the info about function name, start address and length.
> > > JitCodeMoved is invoked when GC moves code object from one address to
> another,
> > > And JitCodeDeleted is called when the code is deleted.
> > >
> > > Current v8 profiler logs samples along with Jit code events in a buffer
> > without
> > > any processing for the sake of performance, just to minimize the profile
> > > skewing.
> > > That is what I wanted to move to the tracing. I.e. the plan is to record
> > samples
> > > and Jit code events as instant trace events and let the client to do the
> > > postprocessing.
> > >
> > > If we want to store the profile data in [1] format, we need to map frame
> > > addresses to functions on the fly. Or alternatively store all the
> information
> > > aside of tracing buffers, then postprocess it when tracing is stopped, and
> > > finally inject samples and frames into the TraceLog. Once the post
> processing
> > is
> > > done, Jit code events are not needed anymore.
> >
> > Thanks for the explanation, that makes things a little clearer. Actually I
> > should've mentioned that perf tracing is also done as a postprocess because
> > anything else would similarly have too much overhead. I think it's fine to
do
> > the same thing here and just record the minimum amount of data we need to
> > reconstruct everything later.
> >
> > I think the important thing is that in the end the sample data format is
> > standard so the UI doesn't need to care where it came from. I guess what I'm
> > unsure about is where the postprocessing step happens? For reference, for
perf
> > it's done by [1] (which isn't very ideal).
> >
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/profile_chro...
>
> AFAICT neither TraceLog nor TracingHostMsg_TraceDataCollected IPC event do
> support anything but traceEvents part of the format.
> The actual format is written in tracing_controller which also currently knows
> nothing about sampling and just writes the traceEvents portion.
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...
>
> So it seems to ok to write jit code events and samples as trace events and
make
> the client, e.g. trace_controller do the post processing. wdyt?
Could you elaborate on this a bit, in particular why we need to do the
processing in trace_controller?
On 2015/01/15 16:03:09, yurys wrote:
> On 2015/01/15 15:43:14, alph wrote:
> > On 2015/01/14 16:02:37, Sami wrote:
> > > On 2015/01/14 15:49:09, alph wrote:
> > > > On 2015/01/14 15:32:45, Sami wrote:
> > > > > That said, it would be nice if the format of the sample itself would
be
> > the
> > > > same
> > > > > even if there's no stack frame (beyond the code name that you've got
> > here?).
> > >
> > > > >
> > > > > For reference, here's what a perf sample contains:
> > > > >
> > > > > {
> > > > > 'cpu': 0, 'tid': 1, 'ts': 1000.0,
> > > > > 'name': 'cycles:HG', 'sf': 3, 'weight': 1
> > > > > }
> > > > >
> > > > > Do you think these jit events would fit into that model?
> > > >
> > > > Ok, let me describe in short the current schema of v8 sampling profiler
> > we're
> > > > trying to move into chromium.
> > > >
> > > > There's a sampler thread that once in a millisecond stops the v8 thread
> and
> > > > invokes a V8 API for a call stack. The callstack is plain array of PC
> > > addresses
> > > > of frames.
> > > > As long as the V8 heap is dynamic, i.e. functions may move around the
heap
> > > > during execution (well, during GC) there are JIT code events. They are
> used
> > to
> > > > map addresses to function names. JitCodeAdded is fired when a new code
> > object
> > > is
> > > > created. It has the info about function name, start address and length.
> > > > JitCodeMoved is invoked when GC moves code object from one address to
> > another,
> > > > And JitCodeDeleted is called when the code is deleted.
> > > >
> > > > Current v8 profiler logs samples along with Jit code events in a buffer
> > > without
> > > > any processing for the sake of performance, just to minimize the profile
> > > > skewing.
> > > > That is what I wanted to move to the tracing. I.e. the plan is to record
> > > samples
> > > > and Jit code events as instant trace events and let the client to do the
> > > > postprocessing.
> > > >
> > > > If we want to store the profile data in [1] format, we need to map frame
> > > > addresses to functions on the fly. Or alternatively store all the
> > information
> > > > aside of tracing buffers, then postprocess it when tracing is stopped,
and
> > > > finally inject samples and frames into the TraceLog. Once the post
> > processing
> > > is
> > > > done, Jit code events are not needed anymore.
> > >
> > > Thanks for the explanation, that makes things a little clearer. Actually I
> > > should've mentioned that perf tracing is also done as a postprocess
because
> > > anything else would similarly have too much overhead. I think it's fine to
> do
> > > the same thing here and just record the minimum amount of data we need to
> > > reconstruct everything later.
> > >
> > > I think the important thing is that in the end the sample data format is
> > > standard so the UI doesn't need to care where it came from. I guess what
I'm
> > > unsure about is where the postprocessing step happens? For reference, for
> perf
> > > it's done by [1] (which isn't very ideal).
> > >
> > > [1]
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/tools/profile_chro...
> >
> > AFAICT neither TraceLog nor TracingHostMsg_TraceDataCollected IPC event do
> > support anything but traceEvents part of the format.
> > The actual format is written in tracing_controller which also currently
knows
> > nothing about sampling and just writes the traceEvents portion.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/tr...
> >
> > So it seems to ok to write jit code events and samples as trace events and
> make
> > the client, e.g. trace_controller do the post processing. wdyt?
>
> Could you elaborate on this a bit, in particular why we need to do the
> processing in trace_controller?
Both TraceLog and TracingHostMsg_TraceDataCollected have quite simple APIs.
They support only the trace events part of the format which is passed as a
sequence of JSON-ized trace events separated with commas.
The actual format described in
https://docs.google.com/document/d/1CvAClvFfyA5R-PhYUmn5OOQtYMH4h6I0nSsKchNAy...
is written in trace controller. Although the trace controller itself currently
supports only the traceEvents part of the format. Both samples and stackFrames
are not supported.
If we implement post processing on the TraceLog side, it would require changing
(complicating) its API as well as the IPC to be able to pass samples and frames.
P.S. I'm curious what was the rationale behind adding two extra fields (samples,
stackFrames). The list of trace events is already powerful enough to contain all
the data, e.g. the samples could be written as regular instant (or sample) trace
events and the frames data could be incorporated into metadata events.
talked offline with yurys --- i think it seems logical to postprocess offline.
that would imply
- we spec the format of the code motion events
- the code motion events are ph=M events, and get buffered differently so that
they are never discarded when in ringbuffering mode etc
- we need to figure out whether the code motion events are recorded even when
tracing is off, or if we do a big dump of code motion events when tracing turns
on]
- the sample events have pc samples and we update the trace event format doc to
describe this format
- someone agrees to implement an importer for this new format in trace-viewer at
some point [maybe fady?]
- we reconcile the current {traceEvents:[], stackFrames: [], samples: []} format
with the one that is being described here. may be that we do have two formats,
but we should have a clear understanding of why. document the rationale if there
is one in trace event format spec doc?
> - someone agrees to implement an importer for this new format in trace-viewer at > some point [maybe fady?] Once we have some data using the new format, I will write the importer for trace-viewer.
On 2015/01/15 18:49:24, nduca wrote:
> talked offline with yurys --- i think it seems logical to postprocess offline.
>
> that would imply
> - we spec the format of the code motion events
> - the code motion events are ph=M events, and get buffered differently so
that
> they are never discarded when in ringbuffering mode etc
The current patch just disables sampling if it's in the ringbuffer mode. I could
address that in upcoming patches.
Btw, are meta events never discarded?
> - we need to figure out whether the code motion events are recorded even when
> tracing is off, or if we do a big dump of code motion events when tracing
turns
> on]
I see the following cons against the former one:
* the jit events buffer will bloat to an insane size if it is recorded all the
time.
* there will be a performance impact even when non recording.
The latter seems to be the only option.
> - the sample events have pc samples and we update the trace event format doc
to
> describe this format
Sure. Btw how do we update the doc. I don't have permissions to edit it.
> - someone agrees to implement an importer for this new format in trace-viewer
at
> some point [maybe fady?]
>
> - we reconcile the current {traceEvents:[], stackFrames: [], samples: []}
format
> with the one that is being described here. may be that we do have two formats,
> but we should have a clear understanding of why. document the rationale if
there
> is one in trace event format spec doc?
On 2015/01/16 at 17:43:40, alph wrote: > > - the sample events have pc samples and we update the trace event format doc to > > describe this format > > Sure. Btw how do we update the doc. I don't have permissions to edit it. > You now have edit access.
On 2015/01/16 17:43:40, alph wrote: > On 2015/01/15 18:49:24, nduca wrote: > > talked offline with yurys --- i think it seems logical to postprocess offline. > > > > that would imply > > - we spec the format of the code motion events > > - the code motion events are ph=M events, and get buffered differently so > that > > they are never discarded when in ringbuffering mode etc > > The current patch just disables sampling if it's in the ringbuffer mode. I could > address that in upcoming patches. > Btw, are meta events never discarded? > No, meta events are written in the same buffer as regular events and can be discarded. We'll need to implement a separate buffer for them. > > - we need to figure out whether the code motion events are recorded even when > > tracing is off, or if we do a big dump of code motion events when tracing > turns > > on] > > I see the following cons against the former one: > * the jit events buffer will bloat to an insane size if it is recorded all the > time. > * there will be a performance impact even when non recording. > > The latter seems to be the only option. > Yeah, it already works this way in v8 sampling profiler: when profiling is starting we dump code creation events for all existing code objects.
On 2015/01/19 06:17:59, yurys wrote: > On 2015/01/16 17:43:40, alph wrote: > > On 2015/01/15 18:49:24, nduca wrote: > > > talked offline with yurys --- i think it seems logical to postprocess > offline. > > > > > > that would imply > > > - we spec the format of the code motion events > > > - the code motion events are ph=M events, and get buffered differently so > > that > > > they are never discarded when in ringbuffering mode etc > > > > The current patch just disables sampling if it's in the ringbuffer mode. I > could > > address that in upcoming patches. > > Btw, are meta events never discarded? > > > No, meta events are written in the same buffer as regular events and can be > discarded. We'll need to implement a separate buffer for them. There's a theoretical problem with this approach though. If a meta event and a sample event have the same timestamp it's impossible to restore their order without an additional info. However we can assume the probability of this event is zero. > > > > - we need to figure out whether the code motion events are recorded even > when > > > tracing is off, or if we do a big dump of code motion events when tracing > > turns > > > on] > > > > I see the following cons against the former one: > > * the jit events buffer will bloat to an insane size if it is recorded all the > > time. > > * there will be a performance impact even when non recording. > > > > The latter seems to be the only option. > > > Yeah, it already works this way in v8 sampling profiler: when profiling is > starting we dump code creation events for all existing code objects.
On 2015/01/15 18:49:24, nduca wrote:
> talked offline with yurys --- i think it seems logical to postprocess offline.
>
> that would imply
> - we spec the format of the code motion events
> - the code motion events are ph=M events, and get buffered differently so
that
> they are never discarded when in ringbuffering mode etc
> - we need to figure out whether the code motion events are recorded even when
> tracing is off, or if we do a big dump of code motion events when tracing
turns
> on]
> - the sample events have pc samples and we update the trace event format doc
to
> describe this format
> - someone agrees to implement an importer for this new format in trace-viewer
at
> some point [maybe fady?]
>
> - we reconcile the current {traceEvents:[], stackFrames: [], samples: []}
format
> with the one that is being described here. may be that we do have two formats,
> but we should have a clear understanding of why. document the rationale if
there
> is one in trace event format spec doc?
I created a doc on the format
https://docs.google.com/document/d/1DDS8UJySywkYZDpmgocGBAmB7jVZJyMvN22f_6wxq...
If we agree on it I'll move it to the main tracing format doc. ptal.
It conforms to the variant A of https://docs.google.com/document/d/1DDS8UJySywkYZDpmgocGBAmB7jVZJyMvN22f_6wxq... ptal
lgtm https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... content/renderer/devtools/v8_sampling_profiler.cc:25: base::snprintf(buffer, sizeof(buffer), "%p", value); It might make sense to share implementation with tracing code: https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... Otherwise IIRC this code will produce different strings on win and linux (different case). https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... content/renderer/devtools/v8_sampling_profiler.cc:48: static void JitCodeEventHandler(const v8::JitCodeEvent* event); HandleJitCodeEvent
https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... content/renderer/devtools/v8_sampling_profiler.cc:91: TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), Using dots seems to be more common then _'s in category names. So, v8.cpu_profile or v8.cpu.profile maybe?
https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... File content/renderer/devtools/v8_sampling_profiler.cc (right): https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... content/renderer/devtools/v8_sampling_profiler.cc:25: base::snprintf(buffer, sizeof(buffer), "%p", value); On 2015/02/24 13:45:47, yurys wrote: > It might make sense to share implementation with tracing code: > > https://code.google.com/p/chromium/codesearch#chromium/src/base/trace_event/t... > > Otherwise IIRC this code will produce different strings on win and linux > (different case). Done. https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... content/renderer/devtools/v8_sampling_profiler.cc:48: static void JitCodeEventHandler(const v8::JitCodeEvent* event); On 2015/02/24 13:45:47, yurys wrote: > HandleJitCodeEvent Done. https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... content/renderer/devtools/v8_sampling_profiler.cc:91: TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), On 2015/02/24 14:40:17, dsinclair wrote: > Using dots seems to be more common then _'s in category names. So, > v8.cpu_profile or v8.cpu.profile maybe? I have an impression that dots are used to form a kind of hierarchy, and '_' is used to split the words, see e.g. blink.image_decoding, blink.user_timing
On 2015/02/24 at 15:28:16, alph wrote: > https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... > content/renderer/devtools/v8_sampling_profiler.cc:91: TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), > On 2015/02/24 14:40:17, dsinclair wrote: > > Using dots seems to be more common then _'s in category names. So, > > v8.cpu_profile or v8.cpu.profile maybe? > > I have an impression that dots are used to form a kind of hierarchy, and '_' is used to split the words, see e.g. blink.image_decoding, blink.user_timing So, in that case, v8.cpu_profile since we already have a v8 category then?
On 2015/02/24 15:29:31, dsinclair wrote: > On 2015/02/24 at 15:28:16, alph wrote: > > > https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... > > content/renderer/devtools/v8_sampling_profiler.cc:91: > TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), > > On 2015/02/24 14:40:17, dsinclair wrote: > > > Using dots seems to be more common then _'s in category names. So, > > > v8.cpu_profile or v8.cpu.profile maybe? > > > > I have an impression that dots are used to form a kind of hierarchy, and '_' > is used to split the words, see e.g. blink.image_decoding, blink.user_timing > > > So, in that case, v8.cpu_profile since we already have a v8 category then? Perhaps, but I'd leave v8.* group for events injected by v8 itself. wdyt?
On 2015/02/24 at 15:35:30, alph wrote: > On 2015/02/24 15:29:31, dsinclair wrote: > > On 2015/02/24 at 15:28:16, alph wrote: > > > > > https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... > > > content/renderer/devtools/v8_sampling_profiler.cc:91: > > TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), > > > On 2015/02/24 14:40:17, dsinclair wrote: > > > > Using dots seems to be more common then _'s in category names. So, > > > > v8.cpu_profile or v8.cpu.profile maybe? > > > > > > I have an impression that dots are used to form a kind of hierarchy, and '_' > > is used to split the words, see e.g. blink.image_decoding, blink.user_timing > > > > > > So, in that case, v8.cpu_profile since we already have a v8 category then? > > Perhaps, but I'd leave v8.* group for events injected by v8 itself. wdyt? I don't think that's a distinction that would come through in the tracing UIs. It's a tracing category related to v8, we'd just have a random set of .'s and _'s of no apparent reason. We especially don't want v8 to have a v8.cpu_profile and a v8_cpu_profile which are different things.
On 2015/02/24 15:37:24, dsinclair wrote: > On 2015/02/24 at 15:35:30, alph wrote: > > On 2015/02/24 15:29:31, dsinclair wrote: > > > On 2015/02/24 at 15:28:16, alph wrote: > > > > > > > > https://codereview.chromium.org/792903003/diff/60001/content/renderer/devtool... > > > > content/renderer/devtools/v8_sampling_profiler.cc:91: > > > TRACE_EVENT_INSTANT1(TRACE_DISABLED_BY_DEFAULT("v8_cpu_profile"), > > > > On 2015/02/24 14:40:17, dsinclair wrote: > > > > > Using dots seems to be more common then _'s in category names. So, > > > > > v8.cpu_profile or v8.cpu.profile maybe? > > > > > > > > I have an impression that dots are used to form a kind of hierarchy, and > '_' > > > is used to split the words, see e.g. blink.image_decoding, blink.user_timing > > > > > > > > > So, in that case, v8.cpu_profile since we already have a v8 category then? > > > > Perhaps, but I'd leave v8.* group for events injected by v8 itself. wdyt? > > > > I don't think that's a distinction that would come through in the tracing UIs. > It's a tracing category related to v8, we'd just have a random set of .'s and > _'s of no apparent reason. We especially don't want v8 to have a v8.cpu_profile > and a v8_cpu_profile which are different things. Ok. done.
lgtm. thanks.
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, yurys@chromium.org Link to the patchset: https://codereview.chromium.org/792903003/#ps120001 (title: "Rename category v8_cpu_profile -> v8.cpu_profile")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/792903003/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_rel on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, dsinclair@chromium.org, yurys@chromium.org Link to the patchset: https://codereview.chromium.org/792903003/#ps140001 (title: "Get rid of CurrentThreadHandle")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/792903003/140001
The CQ bit was checked by alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, dsinclair@chromium.org, yurys@chromium.org Link to the patchset: https://codereview.chromium.org/792903003/#ps160001 (title: "Fix the test under Debug mode.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/792903003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/c8094f8f44f4ad509c3fe003dfcee210beb77216 Cr-Commit-Position: refs/heads/master@{#318025} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
