DevTools: Show GPU utilization bar on timeline

DevTools: Show GPU utilization bar on timeline

The patch adds another strip below CPU utilization that
shows GPU process utilization.

BUG=298951
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=161468

[alph, 2013-10-31 09:41:04]

Andrey, Yury,
Could you please take a look. Thanks.

[yurys, 2013-10-31 15:06:52]

lgtm

https://codereview.chromium.org/46663010/diff/1/Source/core/inspector/Timelin...
File Source/core/inspector/TimelineTraceEventProcessor.h (right):

https://codereview.chromium.org/46663010/diff/1/Source/core/inspector/Timelin...
Source/core/inspector/TimelineTraceEventProcessor.h:84:
TimelineTraceEventProcessor(WeakPtr<InspectorTimelineAgent>, InspectorClient*,
bool);
bool collectGpuEvents for better readability?

https://codereview.chromium.org/46663010/diff/1/Source/core/platform/Platform...
File Source/core/platform/PlatformInstrumentation.cpp (right):

https://codereview.chromium.org/46663010/diff/1/Source/core/platform/Platform...
Source/core/platform/PlatformInstrumentation.cpp:46: const char
PlatformInstrumentation::GpuTaskRendererPid[] = "renderer_pid";
Please be consistent with the event naming style above.

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
File Source/devtools/front_end/TimelineOverviewPane.js (right):

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelineOverviewPane.js:565: ++numberOfStrips;
Sounds like unrelated change.

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelinePanel.js:140: this._cpuBarsElement.style.top =
"1px";
I'd rather do this in timelinePanel.css since you already provide there top and
height for the case when GPU bars are off.

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelinePanel.js:1013: if (!container)
This is always false.

[alph, 2013-10-31 16:57:59]

https://codereview.chromium.org/46663010/diff/1/Source/core/inspector/Timelin...
File Source/core/inspector/TimelineTraceEventProcessor.h (right):

https://codereview.chromium.org/46663010/diff/1/Source/core/inspector/Timelin...
Source/core/inspector/TimelineTraceEventProcessor.h:84:
TimelineTraceEventProcessor(WeakPtr<InspectorTimelineAgent>, InspectorClient*,
bool);
On 2013/10/31 15:06:52, Yury Semikhatsky wrote:
> bool collectGpuEvents for better readability?

Done.

https://codereview.chromium.org/46663010/diff/1/Source/core/platform/Platform...
File Source/core/platform/PlatformInstrumentation.cpp (right):

https://codereview.chromium.org/46663010/diff/1/Source/core/platform/Platform...
Source/core/platform/PlatformInstrumentation.cpp:46: const char
PlatformInstrumentation::GpuTaskRendererPid[] = "renderer_pid";
On 2013/10/31 15:06:52, Yury Semikhatsky wrote:
> Please be consistent with the event naming style above.
Done. rendererPid

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
File Source/devtools/front_end/TimelineOverviewPane.js (right):

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelineOverviewPane.js:565: ++numberOfStrips;
On 2013/10/31 15:06:52, Yury Semikhatsky wrote:
> Sounds like unrelated change.

Reverted.

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelinePanel.js:140: this._cpuBarsElement.style.top =
"1px";
On 2013/10/31 15:06:52, Yury Semikhatsky wrote:
> I'd rather do this in timelinePanel.css since you already provide there top
and
> height for the case when GPU bars are off.

Done.

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelinePanel.js:1013: if (!container)
On 2013/10/31 15:06:52, Yury Semikhatsky wrote:
> This is always false.

Removed.

[caseq, 2013-11-01 13:04:20]

https://codereview.chromium.org/46663010/diff/90001/LayoutTests/inspector/tim...
File LayoutTests/inspector/timeline/timeline-enum-stability-expected.txt
(right):

https://codereview.chromium.org/46663010/diff/90001/LayoutTests/inspector/tim...
LayoutTests/inspector/timeline/timeline-enum-stability-expected.txt:16: GpuTask
: "GpuTask"
GPUTask

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.h:86: extern const char GpuTask[];
ditto

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
File Source/core/inspector/TimelineTraceEventProcessor.cpp (right):

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
Source/core/inspector/TimelineTraceEventProcessor.cpp:432: m_gpuTaskRendererPid
= event.asUInt(PlatformInstrumentation::GpuTaskRendererPid);
nit: m_gpuTask(Initiator|Owner)Pid

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
Source/core/inspector/TimelineTraceEventProcessor.cpp:440: RefPtr<JSONObject>
gpuEvent = TimelineRecordFactory::createBackgroundRecord(m_gpuTaskStart, "gpu");
Use TimelineTraceProcessor::createRecord() instead.

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
File Source/core/inspector/TimelineTraceEventProcessor.h (right):

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
Source/core/inspector/TimelineTraceEventProcessor.h:84:
TimelineTraceEventProcessor(WeakPtr<InspectorTimelineAgent>, InspectorClient*,
bool collectGpuEvents);
do we have to pass this? can we pull it from timeline instead?

https://codereview.chromium.org/46663010/diff/90001/Source/core/platform/Plat...
File Source/core/platform/PlatformInstrumentation.h (right):

https://codereview.chromium.org/46663010/diff/90001/Source/core/platform/Plat...
Source/core/platform/PlatformInstrumentation.h:69: static const char GpuTask[];
Please move these to InspectorInstrumentation.

https://codereview.chromium.org/46663010/diff/90001/Source/devtools/front_end...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/46663010/diff/90001/Source/devtools/front_end...
Source/devtools/front_end/TimelinePanel.js:666: return true;
So this will fire even if we're adding a record outside of selected window? I
don't think this helps performance ;)

https://codereview.chromium.org/46663010/diff/90001/Source/devtools/front_end...
Source/devtools/front_end/TimelinePanel.js:1040: if (typeof this._pidToHue ===
"undefined")
if (!this._pidToHue) ...

[alph, 2013-11-01 16:40:19]

https://codereview.chromium.org/46663010/diff/90001/LayoutTests/inspector/tim...
File LayoutTests/inspector/timeline/timeline-enum-stability-expected.txt
(right):

https://codereview.chromium.org/46663010/diff/90001/LayoutTests/inspector/tim...
LayoutTests/inspector/timeline/timeline-enum-stability-expected.txt:16: GpuTask
: "GpuTask"
On 2013/11/01 13:04:20, caseq wrote:
> GPUTask

Done.

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Ins...
File Source/core/inspector/InspectorTimelineAgent.h (right):

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Ins...
Source/core/inspector/InspectorTimelineAgent.h:86: extern const char GpuTask[];
On 2013/11/01 13:04:20, caseq wrote:
> ditto

Done.

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
File Source/core/inspector/TimelineTraceEventProcessor.cpp (right):

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
Source/core/inspector/TimelineTraceEventProcessor.cpp:432: m_gpuTaskRendererPid
= event.asUInt(PlatformInstrumentation::GpuTaskRendererPid);
On 2013/11/01 13:04:20, caseq wrote:
> nit: m_gpuTask(Initiator|Owner)Pid

Done.

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
Source/core/inspector/TimelineTraceEventProcessor.cpp:440: RefPtr<JSONObject>
gpuEvent = TimelineRecordFactory::createBackgroundRecord(m_gpuTaskStart, "gpu");
On 2013/11/01 13:04:20, caseq wrote:
> Use TimelineTraceProcessor::createRecord() instead.

Done.

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
File Source/core/inspector/TimelineTraceEventProcessor.h (right):

https://codereview.chromium.org/46663010/diff/90001/Source/core/inspector/Tim...
Source/core/inspector/TimelineTraceEventProcessor.h:84:
TimelineTraceEventProcessor(WeakPtr<InspectorTimelineAgent>, InspectorClient*,
bool collectGpuEvents);
On 2013/11/01 13:04:20, caseq wrote:
> do we have to pass this? can we pull it from timeline instead?

Done.

https://codereview.chromium.org/46663010/diff/90001/Source/core/platform/Plat...
File Source/core/platform/PlatformInstrumentation.h (right):

https://codereview.chromium.org/46663010/diff/90001/Source/core/platform/Plat...
Source/core/platform/PlatformInstrumentation.h:69: static const char GpuTask[];
On 2013/11/01 13:04:20, caseq wrote:
> Please move these to InspectorInstrumentation.

Done.

https://codereview.chromium.org/46663010/diff/90001/Source/devtools/front_end...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/46663010/diff/90001/Source/devtools/front_end...
Source/devtools/front_end/TimelinePanel.js:666: return true;
On 2013/11/01 13:04:20, caseq wrote:
> So this will fire even if we're adding a record outside of selected window? I
> don't think this helps performance ;)

Fixed.

https://codereview.chromium.org/46663010/diff/90001/Source/devtools/front_end...
Source/devtools/front_end/TimelinePanel.js:1040: if (typeof this._pidToHue ===
"undefined")
On 2013/11/01 13:04:20, caseq wrote:
> if (!this._pidToHue) ...

Done.

[alph, 2013-11-01 18:53:20]

ptal

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/46663010/diff/1/Source/devtools/front_end/Tim...
Source/devtools/front_end/TimelinePanel.js:1013: if (!container)
On 2013/10/31 16:58:00, alph wrote:
> On 2013/10/31 15:06:52, Yury Semikhatsky wrote:
> > This is always false.
> 
> Removed.

This might be true if the user is enabled the experiment after the panel has
been created (without the gpu bars container). So reverting it back.

[caseq, 2013-11-05 14:26:57]

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
File Source/core/inspector/TimelineTraceEventProcessor.cpp (right):

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
Source/core/inspector/TimelineTraceEventProcessor.cpp:185:
registerHandler(InstrumentationEvents::GpuTask, TRACE_EVENT_PHASE_BEGIN,
&TimelineTraceEventProcessor::onGpuTaskBegin);
How about s/Gpu/GPU/ globally on the patch?

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
Source/core/inspector/TimelineTraceEventProcessor.cpp:431:
data->setString("name", event.name());
Do we really want to expose this?

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
Source/core/inspector/TimelineTraceEventProcessor.cpp:432:
data->setNumber("ownerPid",
event.asUInt(InstrumentationEventArguments::OwnerPid));
Please extract the above lines into a TimelineRecordFactory method.

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
Source/devtools/front_end/TimelinePanel.js:1007: * @param {Element} container
?Element I guess?

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
File Source/devtools/front_end/timelinePanel.css (right):

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
Source/devtools/front_end/timelinePanel.css:543: flex-flow: column nowrap;
consider using .vbox

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
Source/devtools/front_end/timelinePanel.css:551: flex: 1 1 auto;
flex: auto;

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
Source/devtools/front_end/timelinePanel.css:558: top: 1px;
can we use margins instead?

[alph, 2013-11-05 14:59:16]

ptal

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
File Source/core/inspector/TimelineTraceEventProcessor.cpp (right):

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
Source/core/inspector/TimelineTraceEventProcessor.cpp:185:
registerHandler(InstrumentationEvents::GpuTask, TRACE_EVENT_PHASE_BEGIN,
&TimelineTraceEventProcessor::onGpuTaskBegin);
On 2013/11/05 14:26:58, caseq wrote:
> How about s/Gpu/GPU/ globally on the patch?

I'd like to keep it as "Gpu" taking into account other usage frequencies:

$ grep -r "Gpu" content/* | grep -v "#" | grep -v "//" | wc -l
2840
$ grep -r "GPU" content/* | grep -v "#" | grep -v "//" | grep -v "GPU_" | wc -l
305

wdyt?

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
Source/core/inspector/TimelineTraceEventProcessor.cpp:431:
data->setString("name", event.name());
On 2013/11/05 14:26:58, caseq wrote:
> Do we really want to expose this?
not really. nuked.

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
Source/core/inspector/TimelineTraceEventProcessor.cpp:432:
data->setNumber("ownerPid",
event.asUInt(InstrumentationEventArguments::OwnerPid));
On 2013/11/05 14:26:58, caseq wrote:
> Please extract the above lines into a TimelineRecordFactory method.

The record creation is quite simple already. Also why do you think it is
different from e.g. onPaint?

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
File Source/devtools/front_end/TimelinePanel.js (right):

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
Source/devtools/front_end/TimelinePanel.js:1007: * @param {Element} container
On 2013/11/05 14:26:58, caseq wrote:
> ?Element I guess?

Done.

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
File Source/devtools/front_end/timelinePanel.css (right):

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
Source/devtools/front_end/timelinePanel.css:543: flex-flow: column nowrap;
On 2013/11/05 14:26:58, caseq wrote:
> consider using .vbox

Done.

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
Source/devtools/front_end/timelinePanel.css:551: flex: 1 1 auto;
On 2013/11/05 14:26:58, caseq wrote:
> flex: auto;

Done.

https://codereview.chromium.org/46663010/diff/210002/Source/devtools/front_en...
Source/devtools/front_end/timelinePanel.css:558: top: 1px;
On 2013/11/05 14:26:58, caseq wrote:
> can we use margins instead?

Done.

[alph, 2013-11-05 18:28:58]

ptal

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
File Source/core/inspector/TimelineTraceEventProcessor.cpp (right):

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
Source/core/inspector/TimelineTraceEventProcessor.cpp:185:
registerHandler(InstrumentationEvents::GpuTask, TRACE_EVENT_PHASE_BEGIN,
&TimelineTraceEventProcessor::onGpuTaskBegin);
On 2013/11/05 14:59:16, alph wrote:
> On 2013/11/05 14:26:58, caseq wrote:
> > How about s/Gpu/GPU/ globally on the patch?
> 
> I'd like to keep it as "Gpu" taking into account other usage frequencies:
> 
> $ grep -r "Gpu" content/* | grep -v "#" | grep -v "//" | wc -l
> 2840
> $ grep -r "GPU" content/* | grep -v "#" | grep -v "//" | grep -v "GPU_" | wc
-l
> 305
> 
> wdyt?

Done.

https://codereview.chromium.org/46663010/diff/210002/Source/core/inspector/Ti...
Source/core/inspector/TimelineTraceEventProcessor.cpp:432:
data->setNumber("ownerPid",
event.asUInt(InstrumentationEventArguments::OwnerPid));
On 2013/11/05 14:59:16, alph wrote:
> On 2013/11/05 14:26:58, caseq wrote:
> > Please extract the above lines into a TimelineRecordFactory method.
> 
> The record creation is quite simple already. Also why do you think it is
> different from e.g. onPaint?

Done.

[alph, 2013-11-06 16:26:27]

friendly ping.

[caseq, 2013-11-06 16:40:30]

lgtm

https://codereview.chromium.org/46663010/diff/630001/Source/core/inspector/In...
File Source/core/inspector/InspectorInstrumentation.h (right):

https://codereview.chromium.org/46663010/diff/630001/Source/core/inspector/In...
Source/core/inspector/InspectorInstrumentation.h:141: extern const char
OwnerPId[];
PID

https://codereview.chromium.org/46663010/diff/630001/Source/core/inspector/Ti...
File Source/core/inspector/TimelineRecordFactory.cpp (right):

https://codereview.chromium.org/46663010/diff/630001/Source/core/inspector/Ti...
Source/core/inspector/TimelineRecordFactory.cpp:226: data->setNumber("ownerPId",
ownerPId);
PID

[commit-bot: I haz the power, 2013-11-06 17:03:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alph@chromium.org/46663010/820001

[commit-bot: I haz the power, 2013-11-06 18:22:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alph@chromium.org/46663010/1120001

[commit-bot: I haz the power, 2013-11-06 19:47:18]

Change committed as 161468