Notify DevTools when updating frame in RenderWidget

Notify DevTools when updating frame in RenderWidget

Add a call to WebWidget::instrumentBeginFrame() in
RenderWiddget::doDeferredUpdate() so DevTools can mark
frames on timeline.

Related issue upstream:
https://bugs.webkit.org/show_bug.cgi?id=80127

BUG=none
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126921

[pfeldman, 2012-03-06 16:04:23]

lgtm

[caseq, 2012-03-06 16:17:02]

James or Darin, could you please have a look?

[nduca, 2012-03-06 16:48:18]

Neat.

http://codereview.chromium.org/9592006/diff/1/content/renderer/render_widget.cc
File content/renderer/render_widget.cc (right):

http://codereview.chromium.org/9592006/diff/1/content/renderer/render_widget....
content/renderer/render_widget.cc:848: return;
If we early-return here, you'll generate a false frame marker *when* there is no
raf registered.

Ideally, you would check whether an animation is requested --- if not, then dont
mark the frame until after this branch. If an animation is requested, then mark
the frame begin where you are.

[jamesr, 2012-03-06 18:54:13]

This is for bracketing a frame more accurately, right?  Wouldn't you also want
to know when we're "done" with a frame - or do you just count everything up to
the start of the next frame against the current one?

[caseq, 2012-03-06 18:59:08]

On 2012/03/06 16:48:18, nduca wrote:
> content/renderer/render_widget.cc:848: return;
> If we early-return here, you'll generate a false frame marker *when* there is
no
> raf registered.
> 
> Ideally, you would check whether an animation is requested --- if not, then
dont
> mark the frame until after this branch. If an animation is requested, then
mark the frame begin where you are.

Do you think we only need to show the frames that are caused by
requestAnimationFrame()? My idea was that we use a wider notion of frame and
include refreshes triggered with other means, e.g. timers or  CSS animation like
this:

http://www.webkit.org/blog-files/3d-transforms/poster-circle.html

[jamesr, 2012-03-06 19:00:40]

On 2012/03/06 18:59:08, caseq wrote:
> On 2012/03/06 16:48:18, nduca wrote:
> > content/renderer/render_widget.cc:848: return;
> > If we early-return here, you'll generate a false frame marker *when* there
is
> no
> > raf registered.
> > 
> > Ideally, you would check whether an animation is requested --- if not, then
> dont
> > mark the frame until after this branch. If an animation is requested, then
> mark the frame begin where you are.
> 
> Do you think we only need to show the frames that are caused by
> requestAnimationFrame()? My idea was that we use a wider notion of frame and
> include refreshes triggered with other means, e.g. timers or  CSS animation
like
> this:
> 
> http://www.webkit.org/blog-files/3d-transforms/poster-circle.html


CSS animations are triggered by the animate() call, but I don't think that's
what Nat's point was - with this patch we might hit the instrumentBeginFrame()
call but then not actually produce a frame lower down.

[caseq, 2012-03-06 19:08:37]

On 2012/03/06 18:54:13, jamesr wrote:
> This is for bracketing a frame more accurately, right?

Yes!

> Wouldn't you also want
> to know when we're "done" with a frame - or do you just count everything up to
> the start of the next frame against the current one?

Ideally, yes, we'd like to have frame end markers, so any hints on what really
constitutes the end of frame are much appreciated. I've originally added an
instrumentation call at the end of DoDeferredUpdate(), but it turns out that
there are some actions happening asynchronously (in particular, I spotted at
least style recalculation being deferred). So for the time being we just account
every event that we instrument for the last frame before the event.
We still may want knowing when DoDeferredUpdate is done just to track the CPU
time used per frame more accurately -- does anyone think this would be useful?

[caseq, 2012-03-06 19:17:55]

On 2012/03/06 19:00:40, jamesr wrote:

> CSS animations are triggered by the animate() call, but I don't think that's
> what Nat's point was - with this patch we might hit the instrumentBeginFrame()
> call but then not actually produce a frame lower down.

yep, I got this point, I was just a bit confused by Nat mentioning RAF. So I
guess we just need to move the call to instrumentBeginFrame() past
HasPendingUpdates() check and pass frame_begin_ticks there so we know exact
start time in the timeline -- does this make sense?
(this will make timeline records arrive out of chronological order, but in this
particular case there shouldn't be much trouble with that).

[jamesr, 2012-03-06 19:20:25]

On 2012/03/06 19:17:55, caseq wrote:
> On 2012/03/06 19:00:40, jamesr wrote:
> 
> > CSS animations are triggered by the animate() call, but I don't think that's
> > what Nat's point was - with this patch we might hit the
instrumentBeginFrame()
> > call but then not actually produce a frame lower down.
> 
> yep, I got this point, I was just a bit confused by Nat mentioning RAF. So I
> guess we just need to move the call to instrumentBeginFrame() past
> HasPendingUpdates() check and pass frame_begin_ticks there so we know exact
> start time in the timeline -- does this make sense?
> (this will make timeline records arrive out of chronological order, but in
this
> particular case there shouldn't be much trouble with that).

That sounds better.  Keep in mind frame_begin_ticks is a base::TimeTicks, so
it's not directly comparable to a base::Time or WTF::currentTime()-derived
timestamp.

[caseq, 2012-03-06 19:21:44]

> That sounds better.  Keep in mind frame_begin_ticks is a base::TimeTicks, so
> it's not directly comparable to a base::Time or WTF::currentTime()-derived
> timestamp.

yup, thanks a lot!

[nduca, 2012-03-06 20:46:17]

I think we might be able to keep things to a single call by defining frames
where we dont paint but do call raf as "a frame"

If you do that, then you just decide up-front if you're going to call animate()
or not [you need check the floor time etc]. If you do, then its a frame, no
questions asked.  If the animate() isn't going to be called, then the frame
begins only when we get below that early out condition from before.

[caseq, 2012-03-14 16:34:23]

I played a bit with the idea of calling instrumentBeginFrame() after we bail out
upon HasPendingUpdate() check, and we did not like the idea of having timeline
events in a non-chronological order. So here's a different approach: we leave
instrumentBeginFrame() before animate/layout, then add instrumentCancelFrame()
in case we choose to return early. In the timeline backend, we  drop the
BeginFrame event upon receiving CancelFrame event iff there were no other events
between them. This should have effect consistent with Nat's proposal, i.e.
consider it a frame if we had RAF (but with no additional logic in
RenderWidget). The upstream change is here:
https://bugs.webkit.org/show_bug.cgi?id=80994

[jamesr, 2012-03-14 18:12:41]

This seems fine, LGTM

[nduca, 2012-03-14 19:13:27]

I'm not sure this is the right way to go.

I dont think we want to cancel frames.

Rather, if we tick animation, that should be a frame. We might not paint, but we
did do work for a "frame." Inspector should simply understand that not every
frame paints.

You wouldn't need cancelFrame --- just before the animateIfNeeded, say
bool didInsturumentBeginFrame = false;
base::Time now = Time::now();
if (willCallAnimate(now)) {
  instrumentBeginFrame(); 
  didInstrumentBeginFrame = true;
}
AnimateIfNeeded(now);
... early out stuff

if(!didInstrumentBeginFrame)
  instrumentBeginFrame

Modfy AnimateIfNeeded to use the passed-in-now in its early out logic.

Where
  willCallAnimate() {
    the early out logic from AnimateIfNeeded code
  }

[caseq, 2012-03-15 15:27:29]

On 2012/03/14 19:13:27, nduca wrote:
> Rather, if we tick animation, that should be a frame. We might not paint, but
we
> did do work for a "frame." Inspector should simply understand that not every
> frame paints.

Totally agree, and this is how it works in my implementation -- we only cancel
frames if there were no other events between BeginFrame and CancelFrame. In case
of RAF, there will be a timeline event for RAF (and possible others resulting
from JS execution), which will flush BeginFrame to front-end and cause
subsequent CancelFrame to be ignored.

> You wouldn't need cancelFrame --- just before the animateIfNeeded, say
[...]
> if(!didInstrumentBeginFrame)
>   instrumentBeginFrame

My concern with this is that we may report BeginFrame from different points in
code which are not necessarily close in time, even in case we don't do
animation. In particular, I'm paranoid about layout -- I reckon it's not
frequent and normally not expensive in the kind of animation-heavy application
that we target, but it's still there. In this example:
http://www.corp.google.com/~nduca/tracing_examples/03_layout.html
we're off by 230-540us (on a z600) because of layout. Moreover, it causes layout
events to appear at the end of a (previous) frame at timeline, which looks
confusing to me.
Are we looking to sacrifice accuracy here in order to avoid exposing
didCancelFrame?

[nduca, 2012-03-15 15:37:51]

On 2012/03/15 15:27:29, caseq wrote:
> In particular, I'm paranoid about layout -- I reckon it's not
> frequent and normally not expensive in the kind of animation-heavy application
> that we target, but it's still there. In this example:
> http://www.corp.google.com/%7Enduca/tracing_examples/03_layout.html
> we're off by 230-540us (on a z600) because of layout. Moreover, it causes
layout
> events to appear at the end of a (previous) frame at timeline, which looks
> confusing to me.
> Are we looking to sacrifice accuracy here in order to avoid exposing
> didCancelFrame?

Oh, darn. You're 110% right and I'm a moron. LGTM and tons of apologies for
holding you up.