Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2396733002:
[profiler] Tracing-based CPU profiler. (Closed)
Description[profiler] Tracing-based CPU profiler.
A new V8 API object v8::TracingCpuProfiler is introduced.
Client can create it on an isolate to enable JS CPU profiles collected
during tracing session.
Once the v8.cpu_profile2 tracing category is enabled the profiler emits
CpuProfile and CpuProfileChunk events with the profile data.
BUG=chromium:406277
Committed: https://crrev.com/4b575dfceff4575ddd5ba0c74089c127d9b68c4f
Cr-Commit-Position: refs/heads/master@{#40054}
Patch Set 1 #
Total comments: 11
Patch Set 2 : addressing comments. #
Total comments: 12
Patch Set 3 : Addressing comments + moving traced-value back to src/tracing #
Messages
Total messages: 31 (15 generated)
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
alph@chromium.org changed reviewers: + caseq@chromium.org, fmeawad@chromium.org, jochen@chromium.org
Folks, could you please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h File src/profiler/traced-value.h (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h... src/profiler/traced-value.h:19: class TracedValue : public ConvertableToTraceFormat { I don't think this file should be in src/profiler and belong to the tracing namespace https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... File src/profiler/tracing-cpu-profiler.cc (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... src/profiler/tracing-cpu-profiler.cc:28: PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profile2"); I would suggest calling it something else so that when we move tracing to use new tracing based profiler we don't have to go back and rename it or keep using v8.cpu_profiler2 Maybe call it: v8.sampler or v8.profile[r]
https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h File src/profiler/traced-value.h (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h... src/profiler/traced-value.h:19: class TracedValue : public ConvertableToTraceFormat { I don't think this file should be in src/profiler and belong to the tracing namespace https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... File src/profiler/tracing-cpu-profiler.cc (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... src/profiler/tracing-cpu-profiler.cc:28: PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profile2"); I would suggest calling it something else so that when we move tracing to use new tracing based profiler we don't have to go back and rename it or keep using v8.cpu_profiler2 Maybe call it: v8.sampler or v8.profile[r]
Description was changed from ========== [profiler] Tracing-based CPU profiler. A new V8 API object v8::TracingCpuProfiler is introduced. Client can create it on an isolate to enable JS CPU profiles collected during tracing session. Once the v8.cpu_profile2 tracing category is enabled the profiler emits CpuProfile and CpuProfileChunk events with the profile data. BUG=chromium:406277 ========== to ========== [profiler] Tracing-based CPU profiler. A new V8 API object v8::TracingCpuProfiler is introduced. Client can create it on an isolate to enable JS CPU profiles collected during tracing session. Once the v8.cpu_profile2 tracing category is enabled the profiler emits CpuProfile and CpuProfileChunk events with the profile data. BUG=chromium:406277 ==========
fmeawad@chromium.org changed reviewers: + lpy@chromium.org
https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h File src/profiler/traced-value.h (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h... src/profiler/traced-value.h:19: class TracedValue : public ConvertableToTraceFormat { On 2016/10/05 15:40:54, fmeawad wrote: > I don't think this file should be in src/profiler and belong to the tracing > namespace I initially placed it into tracing, but there were concerns about people start actively using it. Primiano would like to limit usage of TracedValue as it will require rewrite once tracing v2 arrives. https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... File src/profiler/tracing-cpu-profiler.cc (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... src/profiler/tracing-cpu-profiler.cc:28: PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profile2"); On 2016/10/05 15:40:54, fmeawad wrote: > I would suggest calling it something else so that when we move tracing to use > new tracing based profiler we don't have to go back and rename it or keep using > v8.cpu_profiler2 > Maybe call it: v8.sampler or v8.profile[r] How about v8.cpu_profiler ?
https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h File src/profiler/traced-value.h (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h... src/profiler/traced-value.h:19: class TracedValue : public ConvertableToTraceFormat { On 2016/10/05 15:47:37, alph wrote: > On 2016/10/05 15:40:54, fmeawad wrote: > > I don't think this file should be in src/profiler and belong to the tracing > > namespace > > I initially placed it into tracing, but there were concerns about people start > actively using it. Primiano would like to limit usage of TracedValue as it will > require rewrite once tracing v2 arrives. Can we move it to the profiler domain instead? https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... File src/profiler/tracing-cpu-profiler.cc (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... src/profiler/tracing-cpu-profiler.cc:28: PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profile2"); On 2016/10/05 15:47:37, alph wrote: > On 2016/10/05 15:40:54, fmeawad wrote: > > I would suggest calling it something else so that when we move tracing to use > > new tracing based profiler we don't have to go back and rename it or keep > using > > v8.cpu_profiler2 > > Maybe call it: v8.sampler or v8.profile[r] > > How about v8.cpu_profiler ? Yes, as long as it does not create a conflict
https://codereview.chromium.org/2396733002/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:431: StreamPendingTraceEvents(); So what would be the implications of serializing the tree on sampling interval? Can we still expect samples regular enough so as not to introduce any skewing -- especially on a mobile?
ptal https://codereview.chromium.org/2396733002/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:431: StreamPendingTraceEvents(); On 2016/10/05 16:53:53, caseq wrote: > So what would be the implications of serializing the tree on sampling interval? > Can we still expect samples regular enough so as not to introduce any skewing -- > especially on a mobile? Yeah. I made some measurements and reduced the flush thresholds to 100 samples and 10 nodes. It keeps the load/header ratio for trace event pretty high, while allowing a fast enough serialization (~50μsec per chunk). https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h File src/profiler/traced-value.h (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/traced-value.h... src/profiler/traced-value.h:19: class TracedValue : public ConvertableToTraceFormat { On 2016/10/05 15:56:08, fmeawad wrote: > On 2016/10/05 15:47:37, alph wrote: > > On 2016/10/05 15:40:54, fmeawad wrote: > > > I don't think this file should be in src/profiler and belong to the tracing > > > namespace > > > > I initially placed it into tracing, but there were concerns about people start > > actively using it. Primiano would like to limit usage of TracedValue as it > will > > require rewrite once tracing v2 arrives. > > Can we move it to the profiler domain instead? Oh, yes. Moving it to v8::internal where all the profiler resides. Perhaps I should move all the profiler stuff to v8::internal::profiler but it's for another CL. https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... File src/profiler/tracing-cpu-profiler.cc (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/tracing-cpu-pr... src/profiler/tracing-cpu-profiler.cc:28: PROFILER_TRACE_CATEGORY_ENABLED("v8.cpu_profile2"); On 2016/10/05 15:56:08, fmeawad wrote: > On 2016/10/05 15:47:37, alph wrote: > > On 2016/10/05 15:40:54, fmeawad wrote: > > > I would suggest calling it something else so that when we move tracing to > use > > > new tracing based profiler we don't have to go back and rename it or keep > > using > > > v8.cpu_profiler2 > > > Maybe call it: v8.sampler or v8.profile[r] > > > > How about v8.cpu_profiler ? > > Yes, as long as it does not create a conflict It should not.
lgtm
https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... src/profiler/profile-generator.cc:428: top_down_.pending_nodes_count() >= kNodesFlushCount) add {} https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... src/profiler/profile-generator.cc:470: value->BeginDictionary(); nit: move into BuildNodeValue()? https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... File src/profiler/profile-generator.h (right): https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... src/profiler/profile-generator.h:258: std::vector<const ProfileNode*> RetrievePendingNodes() { nit: s/Retrieve/Take/? https://codereview.chromium.org/2396733002/diff/20001/src/profiler/traced-val... File src/profiler/traced-value.cc (right): https://codereview.chromium.org/2396733002/diff/20001/src/profiler/traced-val... src/profiler/traced-value.cc:14: #ifndef NDEBUG #if DCHECK_IS_ON()? https://codereview.chromium.org/2396733002/diff/20001/src/profiler/traced-val... src/profiler/traced-value.cc:17: #define DCHECK_CURRENT_CONTAINER_IS(x) DCHECK_EQ(x, nesting_stack_.back()) this and the one below -- extract to a common part? https://codereview.chromium.org/2396733002/diff/20001/src/profiler/traced-val... src/profiler/traced-value.cc:49: base::OS::SNPrintF(number_buffer, sizeof(number_buffer), "\\u%04X", nit: s/sizeof/arraysize/?
ptal https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... src/profiler/profile-generator.cc:428: top_down_.pending_nodes_count() >= kNodesFlushCount) On 2016/10/05 18:05:11, caseq wrote: > add {} Done. https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... src/profiler/profile-generator.cc:470: value->BeginDictionary(); On 2016/10/05 18:05:12, caseq wrote: > nit: move into BuildNodeValue()? It doesn't really belongs there, as the call depends on the outer container type. E.g. if a node needs to be inserted into a dictionary rather than array, the call would be BeginDictionary("node"). https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... File src/profiler/profile-generator.h (right): https://codereview.chromium.org/2396733002/diff/20001/src/profiler/profile-ge... src/profiler/profile-generator.h:258: std::vector<const ProfileNode*> RetrievePendingNodes() { On 2016/10/05 18:05:12, caseq wrote: > nit: s/Retrieve/Take/? Done. https://codereview.chromium.org/2396733002/diff/20001/src/profiler/traced-val... File src/profiler/traced-value.cc (right): https://codereview.chromium.org/2396733002/diff/20001/src/profiler/traced-val... src/profiler/traced-value.cc:14: #ifndef NDEBUG On 2016/10/05 18:05:12, caseq wrote: > #if DCHECK_IS_ON()? Done. https://codereview.chromium.org/2396733002/diff/20001/src/profiler/traced-val... src/profiler/traced-value.cc:17: #define DCHECK_CURRENT_CONTAINER_IS(x) DCHECK_EQ(x, nesting_stack_.back()) On 2016/10/05 18:05:12, caseq wrote: > this and the one below -- extract to a common part? Done. https://codereview.chromium.org/2396733002/diff/20001/src/profiler/traced-val... src/profiler/traced-value.cc:49: base::OS::SNPrintF(number_buffer, sizeof(number_buffer), "\\u%04X", On 2016/10/05 18:05:12, caseq wrote: > nit: s/sizeof/arraysize/? Done.
lgtm
https://codereview.chromium.org/2396733002/diff/1/src/profiler/profile-genera... File src/profiler/profile-generator.cc (right): https://codereview.chromium.org/2396733002/diff/1/src/profiler/profile-genera... src/profiler/profile-generator.cc:431: StreamPendingTraceEvents(); On 2016/10/05 17:21:56, alph wrote: > On 2016/10/05 16:53:53, caseq wrote: > > So what would be the implications of serializing the tree on sampling > interval? > > Can we still expect samples regular enough so as not to introduce any skewing > -- > > especially on a mobile? > > Yeah. I made some measurements and reduced the flush thresholds to 100 samples > and 10 nodes. It keeps the load/header ratio for trace event pretty high, while > allowing a fast enough serialization (~50μsec per chunk). Checked it on the mobile. The mean chunk serialization is ~200μsec, which is fine.
The CQ bit was checked by alph@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
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 alph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmeawad@chromium.org Link to the patchset: https://codereview.chromium.org/2396733002/#ps40001 (title: "Addressing comments + moving traced-value back to src/tracing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [profiler] Tracing-based CPU profiler. A new V8 API object v8::TracingCpuProfiler is introduced. Client can create it on an isolate to enable JS CPU profiles collected during tracing session. Once the v8.cpu_profile2 tracing category is enabled the profiler emits CpuProfile and CpuProfileChunk events with the profile data. BUG=chromium:406277 ========== to ========== [profiler] Tracing-based CPU profiler. A new V8 API object v8::TracingCpuProfiler is introduced. Client can create it on an isolate to enable JS CPU profiles collected during tracing session. Once the v8.cpu_profile2 tracing category is enabled the profiler emits CpuProfile and CpuProfileChunk events with the profile data. BUG=chromium:406277 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [profiler] Tracing-based CPU profiler. A new V8 API object v8::TracingCpuProfiler is introduced. Client can create it on an isolate to enable JS CPU profiles collected during tracing session. Once the v8.cpu_profile2 tracing category is enabled the profiler emits CpuProfile and CpuProfileChunk events with the profile data. BUG=chromium:406277 ========== to ========== [profiler] Tracing-based CPU profiler. A new V8 API object v8::TracingCpuProfiler is introduced. Client can create it on an isolate to enable JS CPU profiles collected during tracing session. Once the v8.cpu_profile2 tracing category is enabled the profiler emits CpuProfile and CpuProfileChunk events with the profile data. BUG=chromium:406277 Committed: https://crrev.com/4b575dfceff4575ddd5ba0c74089c127d9b68c4f Cr-Commit-Position: refs/heads/master@{#40054} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4b575dfceff4575ddd5ba0c74089c127d9b68c4f Cr-Commit-Position: refs/heads/master@{#40054} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
