[not for review] Add Draw entries to window Performance Timeline

[not for review] Add Draw entries to window Performance Timeline

[not for review]
Spec: http://www.w3.org/TR/performance-timeline/
"Entries" in the performance timeline can be extended by browser vendors. This CL is to create a type to use for recording timing events when a frame is drawn to the screen. This will be used to let app developers calculate their effective frames per second and time-to-frame events.

BUG=120796
R=nduca@google.com

** For related Blink changes, see https://codereview.chromium.org/390193003/

[enne (OOO), 2014-07-22 20:44:26]

I have a lot of small comments, but the general direction seems legit to me. 
All the plumbing you have to do for this makes me sad, but there's no other way
around it, I think.

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
File cc/debug/performance_draw_timing_counter.cc (right):

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.cc:34: struct DrawEvent
drawEvent(it->first, it->second, timestamp);
style nit: draw_event, no struct

bikeshed: Maybe call this DrawTimingEvent for consistency?

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
File cc/debug/performance_draw_timing_counter.h (right):

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:22: // intelligently compute average
frames per second.
Where does it compute average fps?

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:23: class
PerformanceDrawTimingCounter {
bikeshedding: The word "performance" here is a little bit wordy.  You could
probably just call this DrawTimingCounter to go with DrawTimingSet and
DrawTimingInfo.

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:47: scoped_ptr<DrawTimingSet>
getDrawTimingCounts();
style nit: get_draw_timing_counts

Also, "get" is a bit innocuous of a name for something that seems to do a bunch
of work.  Maybe "calculate" instead of "get"?

You could also skip the allocation and just make this void
calculate_draw_timing_counts(DrawTimingSet* output)

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:49: void clearEvents() {
ring_buffer_.Clear(); }
style nit: clear_events

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:54: void SaveTimeStamp(int frame_id,
int64_t rect_id, base::TimeTicks timestamp);
Is this function used?

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:57: const std::vector<std::pair<int,
int64_t> > &frame_ids);
Did git cl upload complain at you about needing to run git cl format? It should
have fixed this.

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:59: typedef RingBuffer<struct
DrawEvent, 4096> RingBufferType;
What happens when this overflows?

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:64: explicit
PerformanceDrawTimingCounter();
style nit: explicit only on single-arg constructors

https://codereview.chromium.org/397443002/diff/60001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/397443002/diff/60001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:86: typedef std::vector<std::pair<int64_t, gfx::Rect> >
DrawFrameRequestRectsType;
This could maybe go in LayerTreeHostCommon too with the other DrawTiming types
so you don't have to define it in both Layer and LayerImpl?

bikeshed: You could also maybe call it DrawTimingRequests to match other
DrawTiming* classes?

https://codereview.chromium.org/397443002/diff/60001/cc/layers/paint_properti...
File cc/layers/paint_properties.h (right):

https://codereview.chromium.org/397443002/diff/60001/cc/layers/paint_properti...
cc/layers/paint_properties.h:25: std::vector<std::pair<int, int64_t> >
draw_frame_request_ids;
This looks unused.  I'm not sure you should be changing PaintProperties anyway;
these requests and rects should all live on layers.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1084:
client_->RecordDrawTiming(info.draws[i].rect_id, info.draws[i].timestamps);
Can you help me understand what the eventual API is supposed to be? In some
places in the code you are using source frame number and others you're using
timestamps.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:232: void ApplyScrollAndScale(ScrollAndScaleSet*
info);
?

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:858: const std::vector<std::pair<int64_t,
gfx::Rect> > &vec =
This block of code could probably just get moved into AppendQuadsForalyer.

Also, if you want this to be real, you probably need to store these rects on
quads themselves so that the browser compositor can tell you when they're
actually shown on screen.  I remember discussing that this is a followup, so
maybe you could leave a TODO so it's more obvious that this is not the end
state.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:860: if (vec.size()) {
style nit: early out here and continue rather than indenting the whole block
below

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:861: gfx::Rect viewport =
it->visible_content_rect();
bikeshed: This isn't the viewport, it's the viewport projected into the layer's
content space.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:869: bool intersects =
it->visible_content_rect().Intersects(
visible content rect is in content space (aka layer bounds scaled by
contents_scale()), so you can't compare them directly.  You need to scale one or
the other, take the enclosing content rect to round up to integers, and then
intersect.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:877: uncommitted_draw_frame_ids_.push_back(
This should live on the frame data too and not be a member variable.  Then you
won't have to clear it.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:878:
std::make_pair(active_tree_->source_frame_number(), rect_id));
How does performance timeline handle getting the same rect ids drawn on
different compositor frames from the same source frame number?

Where does this frame number get used? It looks like the only thing that gets
back up to Blink is an id -> [timestamps] mapping?

https://codereview.chromium.org/397443002/diff/60001/content/renderer/composi...
File content/renderer/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/397443002/diff/60001/content/renderer/composi...
content/renderer/compositor_bindings/web_layer_impl.cc:367: gfx::Rect
gfxRect(rects[i].second);
style nit: This file should be in Chrome style, so gfxRect => gfx_rect. 
Actually, you should probably just call it rect or skip the temporary.

https://codereview.chromium.org/397443002/diff/60001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/397443002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:778: const
std::vector<std::pair<int, base::TimeTicks> > &draws) {
style nit: ">& draws" not "> &draws"

Can you git cl format your patch?

https://codereview.chromium.org/397443002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:782:
//widget_->webwidget()->applyScrollAndScale(scroll_delta, page_scale);
?

https://codereview.chromium.org/397443002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/397443002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:39574: +  <int value="477"
label="LegacyFullScreenErrorExemption"/>
Is this accidentally included?

[Mike B, 2014-07-25 23:09:41]

Still working on moving routine from CalculateRenderPasses to AppendQuads, but
the other review comments should be addressed, thanks!

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
File cc/debug/performance_draw_timing_counter.cc (right):

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.cc:34: struct DrawEvent
drawEvent(it->first, it->second, timestamp);
On 2014/07/22 20:44:24, enne wrote:
> style nit: draw_event, no struct
> 
> bikeshed: Maybe call this DrawTimingEvent for consistency?

Done.

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
File cc/debug/performance_draw_timing_counter.h (right):

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:22: // intelligently compute average
frames per second.
It does not. That comment was copied over when I cribbed from
frame_rate_counter.h. I'll clean up those vestiges!

On 2014/07/22 20:44:25, enne wrote:
> Where does it compute average fps?

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:23: class
PerformanceDrawTimingCounter {
On 2014/07/22 20:44:25, enne wrote:
> bikeshedding: The word "performance" here is a little bit wordy.  You could
> probably just call this DrawTimingCounter to go with DrawTimingSet and
> DrawTimingInfo.

Done.

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:47: scoped_ptr<DrawTimingSet>
getDrawTimingCounts();
I think you're right and "get" is probably not appropriate. 
That would suggest it should be CalculateDrawTimingCounts though.


On 2014/07/22 20:44:25, enne wrote:
> style nit: get_draw_timing_counts
> 
> Also, "get" is a bit innocuous of a name for something that seems to do a
bunch
> of work.  Maybe "calculate" instead of "get"?
> 
> You could also skip the allocation and just make this void
> calculate_draw_timing_counts(DrawTimingSet* output)

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:49: void clearEvents() {
ring_buffer_.Clear(); }
On 2014/07/22 20:44:25, enne wrote:
> style nit: clear_events

Done.

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:54: void SaveTimeStamp(int frame_id,
int64_t rect_id, base::TimeTicks timestamp);
Nope. Removed.
On 2014/07/22 20:44:25, enne wrote:
> Is this function used?

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:57: const std::vector<std::pair<int,
int64_t> > &frame_ids);
git cl upload complained about a fair number of style issues, but didn't catch
this one.

On 2014/07/22 20:44:25, enne wrote:
> Did git cl upload complain at you about needing to run git cl format? It
should
> have fixed this.

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:59: typedef RingBuffer<struct
DrawEvent, 4096> RingBufferType;
New stuff fails to get appended and drops.
On 2014/07/22 20:44:25, enne wrote:
> What happens when this overflows?

https://codereview.chromium.org/397443002/diff/60001/cc/debug/performance_dra...
cc/debug/performance_draw_timing_counter.h:64: explicit
PerformanceDrawTimingCounter();
On 2014/07/22 20:44:25, enne wrote:
> style nit: explicit only on single-arg constructors

Done.

https://codereview.chromium.org/397443002/diff/60001/cc/layers/layer_impl.h
File cc/layers/layer_impl.h (right):

https://codereview.chromium.org/397443002/diff/60001/cc/layers/layer_impl.h#n...
cc/layers/layer_impl.h:86: typedef std::vector<std::pair<int64_t, gfx::Rect> >
DrawFrameRequestRectsType;
On 2014/07/22 20:44:25, enne wrote:
> This could maybe go in LayerTreeHostCommon too with the other DrawTiming types
> so you don't have to define it in both Layer and LayerImpl?
> 
> bikeshed: You could also maybe call it DrawTimingRequests to match other
> DrawTiming* classes?

Done.

https://codereview.chromium.org/397443002/diff/60001/cc/layers/paint_properti...
File cc/layers/paint_properties.h (right):

https://codereview.chromium.org/397443002/diff/60001/cc/layers/paint_properti...
cc/layers/paint_properties.h:25: std::vector<std::pair<int, int64_t> >
draw_frame_request_ids;
Yes, that was from an earlier iteration and snuck into this change set somehow.

On 2014/07/22 20:44:25, enne wrote:
> This looks unused.  I'm not sure you should be changing PaintProperties
anyway;
> these requests and rects should all live on layers.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host.cc
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.cc:1084:
client_->RecordDrawTiming(info.draws[i].rect_id, info.draws[i].timestamps);
Both.. but timestamp is the main thing being delivered to the caller. Ultimately
I wanted to be able to provide the source of the event back to JS. Although now
that I'm down into that part of the API it's possible that number is meaningless
to JS and I'll trim it back out. In the meantime it gets passed almost all of
the way through, but is dropped right towards the end of cc before it goes to
blink.

On 2014/07/22 20:44:25, enne wrote:
> Can you help me understand what the eventual API is supposed to be? In some
> places in the code you are using source frame number and others you're using
> timestamps.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host.h:232: void ApplyScrollAndScale(ScrollAndScaleSet*
info);
Someone committed and then uncommitted a change here, it tricked me in the git
conflict resolution screen, it's already fixed in my local repository.
On 2014/07/22 20:44:26, enne wrote:
> ?

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:860: if (vec.size()) {
Hm, to do that I need to make it a function I can return from. Or I can
'continue' on the loop, but that might not be clear to future developers who try
and add code below mine in the loop.
Suggestions?
On 2014/07/22 20:44:26, enne wrote:
> style nit: early out here and continue rather than indenting the whole block
> below

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:861: gfx::Rect viewport =
it->visible_content_rect();
On 2014/07/22 20:44:26, enne wrote:
> bikeshed: This isn't the viewport, it's the viewport projected into the
layer's
> content space.
Gotcha, I'm just creating that variable to print out the contents to stdout. It
won't exist in the actual patch.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:869: bool intersects =
it->visible_content_rect().Intersects(
On 2014/07/22 20:44:26, enne wrote:
> visible content rect is in content space (aka layer bounds scaled by
> contents_scale()), so you can't compare them directly.  You need to scale one
or
> the other, take the enclosing content rect to round up to integers, and then
> intersect.

Done.

https://codereview.chromium.org/397443002/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl.cc:878:
std::make_pair(active_tree_->source_frame_number(), rect_id));
It gets 2 entries that have the same source frame but different timestamps.
That's ok.

The source frame number is not actually used, in the current implementation
though.

On 2014/07/22 20:44:26, enne wrote:
> How does performance timeline handle getting the same rect ids drawn on
> different compositor frames from the same source frame number?
> 
> Where does this frame number get used? It looks like the only thing that gets
> back up to Blink is an id -> [timestamps] mapping?

https://codereview.chromium.org/397443002/diff/60001/content/renderer/composi...
File content/renderer/compositor_bindings/web_layer_impl.cc (right):

https://codereview.chromium.org/397443002/diff/60001/content/renderer/composi...
content/renderer/compositor_bindings/web_layer_impl.cc:367: gfx::Rect
gfxRect(rects[i].second);
On 2014/07/22 20:44:26, enne wrote:
> style nit: This file should be in Chrome style, so gfxRect => gfx_rect. 
> Actually, you should probably just call it rect or skip the temporary.

Done.

https://codereview.chromium.org/397443002/diff/60001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/397443002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:778: const
std::vector<std::pair<int, base::TimeTicks> > &draws) {
On 2014/07/22 20:44:26, enne wrote:
> style nit: ">& draws" not "> &draws"
> 
> Can you git cl format your patch?

Done.

https://codereview.chromium.org/397443002/diff/60001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:782:
//widget_->webwidget()->applyScrollAndScale(scroll_delta, page_scale);
On 2014/07/22 20:44:26, enne wrote:
> ?

Done.

https://codereview.chromium.org/397443002/diff/60001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/397443002/diff/60001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:39574: +  <int value="477"
label="LegacyFullScreenErrorExemption"/>
No, this happens when I git pull third_party/WebKit. Part of the process has me
regenerate this file.
On 2014/07/22 20:44:26, enne wrote:
> Is this accidentally included?

[Mike B, 2014-12-04 21:42:37]

Patchset #11 (id:200001) has been deleted