New issue
Advanced search Search tips

Issue 305863 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2013
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

No frame markers in devtools even when there is an animation running

Project Member Reported by nduca@chromium.org, Oct 10 2013

Issue description

This page has fully accelerated animations:
http://www.webkit.org/blog-files/3d-transforms/poster-circle.html

This page fast-scrolls:
http://en.wikipedia.org/wiki/List_of_Pok%C3%A9mon

If the platform has threaded compositing, e.g. mostly everywhere these days, devtools wont show any frames, so people are getting [rightly] confused thinking that its actually worse. This then causes them to [incorrectly] try out direct-animation of .style because then it looks right in the timeline.

This isn't impl-side specific, really.
 

Comment 1 by nduca@chromium.org, Oct 10 2013

When the compositor thread is on, what about us driving the frame markers from cc::Scheduler::BeginFrame()? That's the actual vsync signal... it runs on the impl thread, but its the thing that will cause the main thread frame markers to be created anyway... the nice property of this one is that the Scheduler::BeginFrame is called even when the main thread is idle and a css animation is running.

Comment 2 by nduca@chromium.org, Oct 11 2013

Labels: Hotlist-Jank
@caseq: ping?

Comment 4 by caseq@google.com, Oct 16 2013

Status: Started
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 22 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=160160

------------------------------------------------------------------------
r160160 | caseq@chromium.org | 2013-10-22T09:01:45.486722Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/TimelineTraceEventProcessor.cpp?r1=160160&r2=160159&pathrev=160160
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/TimelineTraceEventProcessor.h?r1=160160&r2=160159&pathrev=160160

DevTools: process trace events from background threads by a timed task.

We used to process pending background events upon BeginFrame, which
was a hack and does not work if main thread frames are not fired.
We should post a main thread task upon adding new events instead.

BUG= 305863 
R=pfeldman@chromium.org

Review URL: https://codereview.chromium.org/27524002
------------------------------------------------------------------------

Comment 6 by caseq@chromium.org, Oct 23 2013

I've been playing with various approaches to this for some time, and so far can't seem to find a good visualization for the timeline overview. Here are some concerns:

- We can't just switch to showing only impl-side frames in the overview, as I presume these would be mostly even and it would defy the purpose of frame overview, which is to expose the offending long frames on the main thread that are caused by expensive JS, DOM etc.

- We can show main frame threads where there are some and fall back to showing impl-side frames when there are no main-thread frames. This mostly resolves the above problem, except for cases when there are some long chunks of main-thread work not related to a frame (consider an expensive setTimeout() callback or some event handler). These will produce a long bar on the Timeline and a bunch of even and short background frames. If the activity has caused invalidations (e.g. style recalc or relayout), this will be followed by a main-thread frame, perhaps rather short one. So  the problem is that we've spent quite some time on the main thread, but it goes unaccounted for in the overview, as all frames are short.

I wonder if we should only expose impl-side frames if there's no instrumented main thread activity at all -- yet this may result in somewhat inconsistent and difficult to understand graph.

Comment 7 by nduca@chromium.org, Oct 23 2013

Maybe we should show both threads then, e.g two parallel bar charts. IE does this to great effect. I think it'd be easier for users to understand anyway.

Comment 8 by nduca@chromium.org, Oct 31 2013

Cc: pfeldman@chromium.org
We need to start moving this forward somehow, I just spent a day trying to use devtools to understand jank and in painting limited cases its baffling. Lets go with two parallel graphs, one for the compositor thread and one for the main thread, unless there are no better ideas outstanding? I understand the desire to present a simplified view to the user, but we've held this bug for 3mo hoping for something better so I think we need to just make a hard call here.
Labels: M-32
This is highest priority on rendering performance for us, is planned, wip and was going to land by the branch date.
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 1 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161147

------------------------------------------------------------------------
r161147 | caseq@chromium.org | 2013-11-01T08:58:01.206882Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/TimelineRecordFactory.h?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/WebDevToolsAgentImpl.h?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/Internals.cpp?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/InspectorController.cpp?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/Internals.h?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/InspectorTimelineAgent.cpp?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/testing/Internals.idl?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/TimelineRecordFactory.cpp?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/WebDevToolsAgentImpl.cpp?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/public/web/WebDevToolsAgent.h?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/InspectorController.h?r1=161147&r2=161146&pathrev=161147
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/InspectorTimelineAgent.h?r1=161147&r2=161146&pathrev=161147

DevTools: add ids to compositor frames

This adds frameId to WebDevToolsAgent::didBeginFrame(), so we can
associate impl-side activity with the main thread frame.

BUG= 305863 
R=pfeldman@chromium.org

Review URL: https://codereview.chromium.org/54273005
------------------------------------------------------------------------
Here's a screenshot of proposed UI. Impl-side frames are at the bottom.
impl-side-frames.png
64.0 KB View Download
To my brain, this seems kinda backwards. We're still maintaining 60fps GPU-wise, so the 30fps bar across the top seems misplaced. 
Should we show the 60fps and 30fps lines against the main thread in this view?

We definitely want to visualize the cost of various operations, but we must still show a realistic picture of the jankiness of the page.
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 5 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161341

------------------------------------------------------------------------
r161341 | caseq@chromium.org | 2013-11-05T15:11:19.628499Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/TimelineOverviewPane.js?r1=161341&r2=161340&pathrev=161341
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/timelinePanel.css?r1=161341&r2=161340&pathrev=161341

Replace hard-coded pixel sized adjustments of overview items in CSS with flex, so we can play with overview size.

BUG= 305863 
R=pfeldman@chromium.org

Review URL: https://codereview.chromium.org/59863002
------------------------------------------------------------------------
Wrt #11/#12, this is a good step forward. I sorta get what Paul is concerned about, but this is so much clearer than before, I'm thrilled at even this. :)
caseq would have another screenshot, but what I suggested was roughly this:
Screen Shot 2013-11-06 at 10.01.59 AM.png
21.9 KB View Download
To #12 -- the FPS line is actually shown for the main thread frames here. We don't show it for the impl-side because of lack of space and to avoid confusion. The impl-side is smaller because it's expected to be way more even and not so interesting for the users (I think there's not a lot you can do if it goes janky(*), it's there primarily as a heart bit for the cases when nothing happens on the main thread).

I hope this will be somewhat less confusing if I explain the demo being inspected on this particular screenshot -- it has a CSS animation _and_ a similar main thread animation that makes all sort of stupid things from time to time (loops for 40ms, uses setTimeout() instead of rAF). So we keep having 60 fps impl-side frames that provide one smoothly animated object and a series of irregular main thread frames that are responsible for a another janky animation.

(*) We don't account impl-side paint to any overview frames yet; we plan to include this into impl-side frames.
Latest screenshot from our joint effort with Pavel.
Screenshot from 2013-11-06 18:24:52.png
10.2 KB View Download

Comment 18 by kareng@google.com, Nov 8 2013

Labels: -M-32 M-33 MovedFrom-32
Moving all non essential bugs to the next Milestone.
Project Member

Comment 19 by bugdroid1@chromium.org, Nov 8 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=161634

------------------------------------------------------------------------
r161634 | caseq@chromium.org | 2013-11-08T17:44:48.162410Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/TimelineOverviewPane.js?r1=161634&r2=161633&pathrev=161634

Refactor TimelineOverview to make squeezing impl-side frames easier

This makes some of TimelineFrameOverview members easier to reuse
by removing side effects and dependency on the class state.

BUG= 305863 

Review URL: https://codereview.chromium.org/59983002
------------------------------------------------------------------------
Project Member

Comment 20 by bugdroid1@chromium.org, Nov 12 2013

------------------------------------------------------------------------
r234469 | caseq@chromium.org | 2013-11-12T09:49:23.934016Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_widget.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_impl.cc?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/gpu/render_widget_compositor.cc?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/ui/compositor/compositor.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_view_impl.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/gpu/render_widget_compositor.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/test/layer_tree_test.cc?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/layer_tree_host_client.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/layer_tree_host.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/browser/renderer_host/compositor_impl_android.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/test/web_layer_tree_view_impl_for_testing.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/test/fake_layer_tree_host_client.h?r1=234469&r2=234468&pathrev=234469
   M http://src.chromium.org/viewvc/chrome/trunk/src/content/renderer/render_widget.cc?r1=234469&r2=234468&pathrev=234469

Propagate source_frame_number from LTH to DevTools

This plumbs source_frame_number to WebDevToolsAgent::didBeginFrame(),
so we can associate impl-side activity with the main frame.

BUG= 305863 

Related blink-side issue: https://codereview.chromium.org/54273005/

R=jamesr@chromium.org, nduca@chromium.org, sky@chromium.org

Review URL: https://codereview.chromium.org/54493003
------------------------------------------------------------------------
Labels: Cr-Platform-DevTools
Project Member

Comment 22 by bugdroid1@chromium.org, Dec 2 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=163000

------------------------------------------------------------------------
r163000 | caseq@chromium.org | 2013-12-02T16:46:28.134663Z

Changed paths:
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/inspector/timeline/timeline-frame-controller-expected.txt?r1=163000&r2=162999&pathrev=163000
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/TimelineOverviewPane.js?r1=163000&r2=162999&pathrev=163000
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/timelinePanel.css?r1=163000&r2=162999&pathrev=163000
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/TimelinePresentationModel.js?r1=163000&r2=162999&pathrev=163000
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/TimelineFrameController.js?r1=163000&r2=162999&pathrev=163000
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/inspector/timeline/timeline-frame-controller.html?r1=163000&r2=162999&pathrev=163000
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/TimelineModel.js?r1=163000&r2=162999&pathrev=163000
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/inspector/timeline/timeline-enum-stability-expected.txt?r1=163000&r2=162999&pathrev=163000
   M http://src.chromium.org/viewvc/blink/trunk/Source/devtools/front_end/inspectorCommon.css?r1=163000&r2=162999&pathrev=163000

Timeline: show impl-side frames on the Timeline overview

Expose impl-side frames as additional row of vertical bars
on top of Timeline frame mode overview.

BUG= 305863 

Review URL: https://codereview.chromium.org/61923003
------------------------------------------------------------------------
Project Member

Comment 23 by bugdroid1@chromium.org, Dec 2 2013

------------------------------------------------------------------------
r238150 | caseq@chromium.org | 2013-12-02T19:23:02.222309Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/test/layer_tree_test.cc?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/debug/devtools_instrumentation.h?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/layer_tree_host.cc?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/layer_tree_host_impl_unittest.cc?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/thread_proxy.cc?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/scheduler/scheduler_unittest.cc?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/layer_tree_host_impl.cc?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/scheduler/scheduler.cc?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/layer_tree_host.h?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/thread_proxy.h?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/layer_tree_host_impl.h?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/trees/tree_synchronizer_unittest.cc?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/scheduler/scheduler.h?r1=238150&r2=238149&pathrev=238150
   M http://src.chromium.org/viewvc/chrome/trunk/src/cc/test/fake_layer_tree_host_impl.cc?r1=238150&r2=238149&pathrev=238150

cc: add DevTools instrumentation for impl-side frames

This adds devtools_instrumentation::didBeginFrame() invoked from
Scheduler::BeginImplFrame() and
devtools_instrumentation::didCommitMainThreadFrame invoked from
LayerTreeHost::FinishCommitOnImplThread(). We use the latter to
associate impl-side frames with main thread ones on the DevTools
side.
Related blink-side patch: https://codereview.chromium.org/59793003/

BUG= 305863 

Review URL: https://codereview.chromium.org/60353002
------------------------------------------------------------------------
 Issue 299164  has been merged into this issue.
Status: Fixed

Sign in to add a comment