Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16848010:
Add LayerTreeHostClient::{will,did}UpdateLayer, report layer updates to DevTools (Closed)
|
Created:
7 years, 6 months ago by caseq Modified:
7 years, 5 months ago CC:
chromium-reviews, jonathan.backer, Ian Vollick, jam, apatrick_chromium, joi+watch-content_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, cc-bugs_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd LayerTreeHostClient::{will,did}UpdateLayer, report layer updates to DevTools
This adds instrumentation methods that mark layer updates. Note that we don't use
trace event based instrumentation, as we only want to receive events related to the
inspected page.
Related blink change: https://codereview.chromium.org/16878004
BUG=244505
R=jamesr@chromium.org, nduca@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=213090
Patch Set 1 : #Patch Set 2 : use trace events, pass cookie obtained from devtools to identify the page #Patch Set 3 : pass tree id to devtools, not vice versa #
Total comments: 2
Patch Set 4 : review comments addressed #
Total comments: 2
Patch Set 5 : rebased for try run #
Total comments: 2
Patch Set 6 : moved initializeLayerTreeView() to WebKit::WebWidgetClient group #
Total comments: 6
Patch Set 7 : review comments addressed, try run #
Messages
Total messages: 29 (0 generated)
Should this use trace_event plumbing? It'd be less exposure via the client interface for sure...
Which is my way of asking for more rationale than you included on the patch description... :) We had a way to obtain traces from only the inspected page somehow, didn't we? Also, you'll get paint events and decodes, amongst other things, inside the update calls, right? Is that a problem?
On 2013/06/15 05:28:05, nduca wrote: > Which is my way of asking for more rationale than you included on the patch > description... :) We had a way to obtain traces from only the inspected page > somehow, didn't we? Err... yes, but only in certain cases so far :) This currently works for stuff associated with content layers, as we can tell those that belong to inspected page by looking at id of layers that we actually paint in WebCore. This does not directly work in case of texture layers that are used by deferred canvas, as we lack any regular WebCore-level events that would let us associate layer id with something that belongs to a page (and I couldn't find any good ways to get them yet). Anyway, I do share your concern about bloating interfaces. Here's a somewhat different approach to get update layer events, this time through trace events: let's pass a cookie from WebDevToolsAgent to LayerTreeHost that would let us check if LayerTreeHost belongs to the inspected page in core (the cookie is just a Page* casted to an int, but it could as well be arbitrary id that we would keep either in InspectorController or InspectorPageAgent). If this goes well, we can eventually replace other instrumentation events that get routed through LayerTreeHostClient, and even more important benefit would be that the impl-side stuff would be easier to instrument).
What if we expose layer ids back to Blink? I think we need something like this eventually, anyway, and now that its just GraphicsLayerChromium... maybe this'd fly?
On 2013/06/18 01:14:32, nduca wrote: > What if we expose layer ids back to Blink? I think we need something like this > eventually, anyway, and now that its just GraphicsLayerChromium... maybe this'd > fly? WebLayer::id() is exposed to Blink.
On 2013/06/18 01:14:32, nduca wrote: > What if we expose layer ids back to Blink? I think we need something like this > eventually, anyway, and now that its just GraphicsLayerChromium... maybe this'd > fly? So we have them -- what was your plan? Looks like we still need a cheap way to check whether a layer belongs to the page or not. We could, perhaps, get that from LayerTreeAgent, but that would add additional dependency to Timeline (not that I'm saying it's bad, just thought getting that through UpdateLayer events is perhaps a simpler way, and associating LayerTreeHost with a page would also allow to simplify other instrumentation a bit).
Associating LTH with a page sounds promising... can we do a similar id scheme?
On 2013/06/18 17:58:26, nduca wrote: > Associating LTH with a page sounds promising... can we do a similar id scheme? Did you have a chance to look at Patch Set 2? ;)
Not sure if this is relevant for this patch or not, but do be aware that a layer can move between different LTH/RenderWidgets over its lifetime.
On 2013/06/18 22:42:11, jamesr wrote: > Not sure if this is relevant for this patch or not, but do be aware that a layer > can move between different LTH/RenderWidgets over its lifetime. Thanks, James! I think this shouldn't be relevant for this patch, but it may be relevant for the way we currently track layers that belong to a page (we keep a map of content layer ids to their root node ids that gets updated during paint or destruction of a layer). Actually, associating LTH with the page may help here as well. Could you please describe some scenarios when layers can change hosts?
On Wed, Jun 19, 2013 at 11:39 AM, <caseq@chromium.org> wrote: > On 2013/06/18 22:42:11, jamesr wrote: > >> Not sure if this is relevant for this patch or not, but do be aware that a >> > layer > >> can move between different LTH/RenderWidgets over its lifetime. >> > > Thanks, James! I think this shouldn't be relevant for this patch, but it > may be > relevant for the way we currently track layers that belong to a page (we > keep a > map of content layer ids to their root node ids that gets updated during > paint > or destruction of a layer). Actually, associating LTH with the page may > help > here as well. Could you please describe some scenarios when layers can > change > hosts? > To help reduce latency: One such scenario is when you drag a tab out of the browser window. James probably knows more. > > > https://codereview.chromium.**org/16848010/<https://codereview.chromium.org/1... >
On 2013/06/19 15:39:54, caseq wrote: > On 2013/06/18 22:42:11, jamesr wrote: > > Not sure if this is relevant for this patch or not, but do be aware that a > layer > > can move between different LTH/RenderWidgets over its lifetime. > > Thanks, James! I think this shouldn't be relevant for this patch, but it may be > relevant for the way we currently track layers that belong to a page (we keep a > map of content layer ids to their root node ids that gets updated during paint > or destruction of a layer). Actually, associating LTH with the page may help > here as well. Could you please describe some scenarios when layers can change > hosts? JS can move DOM nodes and their associated layers between windows/tabs. http://thewildernessdowntown.com/, for instance, creates a number of canvas elements and popups and moves things from one window to another.
I'm a bit meh about having layers have their own ids that cc assigns but the lthi having an id that is provided. Can we normalize to one convention and call it an id? I think probably that would be lthi has an id. And, layer tree host impl should have an id too while you're at it... maybe the mapping of id to your devtools cookie can be done higher up. How did we figure out which page a raster task belonged to anyway? We dont issue any id on those... This basic approach is sound though, relative to layer motion.
On 2013/06/24 19:41:09, nduca wrote: > I'm a bit meh about having layers have their own ids that cc assigns but the > lthi having an id that is provided. Can we normalize to one convention and call > it an id? I think probably that would be lthi has an id. And, layer tree host > impl should have an id too while you're at it... maybe the mapping of id to > your devtools cookie can be done higher up. Here's another patch that just removes cookie and uses LTH id instead (the plan is to sync it over to impl side when we add impl side events). My rationale for "cookie" approach was that since we just need to tell if trace event is related to the inspected page, we could reuse single cookie for multiple objects other than LTH, should the need be, rather than tracking all these objects on the inspector side. > How did we figure out which page a raster task belonged to anyway? We dont issue > any id on those... We track them by layer ids, and we know the "right" layer ids by keeping track of WebCore paints (RenderLayerBacking::paintContents(), to be precise). This does not directly work for canvas layers, since we don't get layer client callbacks anywhere where we have page-like objects. > This basic approach is sound though, relative to layer motion. I'm actually thinking of tracking layer and their ids on the blink/core side eventually, but doing it right is a bit more work.
Thank you! I think this is going in the right direction now, even though it requires some webcore work. I didn't LG because of how id() was implemented, but I'm pretty sure i can lg the next patch. On 2013/06/25 18:06:22, caseq wrote: > Here's another patch that just removes cookie and uses LTH id instead (the plan > is to sync it over to impl side when we add impl side events). > My rationale for "cookie" approach was that since we just need to tell if trace > event is related to the inspected page, we could reuse single cookie for > multiple objects other than LTH, should the need be, rather than tracking all > these objects on the inspector side. I totally see your point. Maybe we can make id generation for all of the compositor more pluggable as a follwoup... the ID primitive is really key to perfmon and tooling, so I'd rather us have one system that we improve upon than a mix of solutions for layers and hosts. The concern I have is explaining "ids" to people -- and having to spend more time explaining why its different for host vs layer. I do a lot of explaining these days, so maybe I'm biased toward reducing my work? :) > We track them by layer ids, and we know the "right" layer ids by keeping track > of WebCore paints (RenderLayerBacking::paintContents(), to be precise). This > does not directly work for canvas layers, since we don't get layer client > callbacks anywhere where we have page-like objects. I'm with you, I think we should move (quickly) to using lthi ids everywhere, even for raster worker cost estimation. Lets file a followup bug? One request, plumb lthi id over now... I'd rather it be consistently everywhere but not being used rather than half-implemented.
https://codereview.chromium.org/16848010/diff/24001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/16848010/diff/24001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:101: instrumentation_cookie_(0) { do you still need the cookie init now? https://codereview.chromium.org/16848010/diff/24001/cc/trees/layer_tree_host.h File cc/trees/layer_tree_host.h (right): https://codereview.chromium.org/16848010/diff/24001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.h:263: uint64 id() const { return reinterpret_cast<uint64>(this); } int64 please, like layer. also, this isn't stable. layer tree host impls could be creatd and deleted. so you're at tcmalloc's mercy that this actually unique. look at layer.cc -- we just have a global s_next_layer_id that we bump in the constructor and use. lets do that here.
On 2013/06/26 05:08:46, nduca wrote: > https://codereview.chromium.org/16848010/diff/24001/cc/trees/layer_tree_host.cc > File cc/trees/layer_tree_host.cc (right): > > https://codereview.chromium.org/16848010/diff/24001/cc/trees/layer_tree_host.... > cc/trees/layer_tree_host.cc:101: instrumentation_cookie_(0) { > do you still need the cookie init now? Nuked, thanks for spotting! > https://codereview.chromium.org/16848010/diff/24001/cc/trees/layer_tree_host.h > File cc/trees/layer_tree_host.h (right): > > https://codereview.chromium.org/16848010/diff/24001/cc/trees/layer_tree_host.... > cc/trees/layer_tree_host.h:263: uint64 id() const { return > reinterpret_cast<uint64>(this); } > int64 please, like layer. Changed to int like layers -- although I wonder, why do we use signed type and then do extra effort to avoid negative values? Homage to unix syscalls? :-) > also, this isn't stable. layer tree host impls could be creatd and deleted. so > you're at tcmalloc's mercy that this actually unique. > > look at layer.cc -- we just have a global s_next_layer_id that we bump in the > constructor and use. lets do that here. Done.
lgtm can you file a followup bug to plumb id to the impl side? https://codereview.chromium.org/16848010/diff/32002/cc/debug/devtools_instrum... File cc/debug/devtools_instrumentation.h (right): https://codereview.chromium.org/16848010/diff/32002/cc/debug/devtools_instrum... cc/debug/devtools_instrumentation.h:44: int layer_id, indentation gone funky https://codereview.chromium.org/16848010/diff/32002/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/16848010/diff/32002/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:952: need_more_updates |= PaintMasksForRenderSurface(*it, queue, stats_ptr); hmm, should we do a followup bug for figuring out this half of the branch....
James, could you please stamp this for content/renderer owners?
https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... content/renderer/render_view_impl.cc:948: if (RenderWidgetCompositor* rwc = compositor()) { this doesn't make sense - how could the compositor already be initialized here? https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... content/renderer/render_view_impl.h:832: virtual void initializeLayerTreeView() OVERRIDE; why is this lowercase? it looks like this isn't grouped correctly with other functions from the interface it inherits from
On 2013/07/19 19:47:47, jamesr wrote: > https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... > content/renderer/render_view_impl.cc:948: if (RenderWidgetCompositor* rwc = > compositor()) { > this doesn't make sense - how could the compositor already be initialized here? This point is actually quite late in the RVI initialization -- it's past webview()->initializeMainFrame(), which results in a lot of stuff including initializing frame loader and Document::styleRecalc() that ultimately ends up creating root layer and enabling compositor, so we get an upcall to initializeLayerTreeView() from WebViewImpl. So yes, we happen to have compositor at this point. It would have been possible to only propagate LTH id in initializaeLayerTreeView(), as it's the only place where RenderWidgetCompositor is created, but if it gets called early (i.e. from initializeMainFrame() as described above), DevToolsAgent won't be there yet. We might save some code by moving DevToolsAgent initialization earlier, but it looks a bit fragile. > https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... > File content/renderer/render_view_impl.h (right): > > https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... > content/renderer/render_view_impl.h:832: virtual void initializeLayerTreeView() > OVERRIDE; > why is this lowercase? it looks like this isn't grouped correctly with other > functions from the interface it inherits from It's in a group of overriden RenderWidget methods, which it is. It's lowercase, 'cause RenderWidget implements it as a WebKit::WebWidgetClient interface method. I'm not sure what's the right thing to do here -- should we really have a separate group for it?
On 2013/07/22 08:44:41, caseq wrote: > On 2013/07/19 19:47:47, jamesr wrote: > > > https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... > > File content/renderer/render_view_impl.h (right): > > > > > https://codereview.chromium.org/16848010/diff/39001/content/renderer/render_v... > > content/renderer/render_view_impl.h:832: virtual void > initializeLayerTreeView() > > OVERRIDE; > > why is this lowercase? it looks like this isn't grouped correctly with other > > functions from the interface it inherits from > > It's in a group of overriden RenderWidget methods, which it is. It's lowercase, > 'cause RenderWidget implements it as a WebKit::WebWidgetClient interface method. > I'm not sure what's the right thing to do here -- should we really have a > separate group for it? Ouch, never mind -- I've overlooked the existing group of WebWidgetClient overrides. Moved the method there.
https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:891: devtools_instrumentation::ScopedLayerTreeTask Just wondering: Does devtools have the ability to use our trace events as well for this kind of purpose? If so, would it make sense to use a TRACE_EVENT for these instead?
https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:891: devtools_instrumentation::ScopedLayerTreeTask On 2013/07/22 15:06:51, danakj wrote: > Just wondering: Does devtools have the ability to use our trace events as well > for this kind of purpose? Yes. > If so, would it make sense to use a TRACE_EVENT for > these instead? devtools_instrumentation::ScopedFooTask classes are wrappers for TRACE_EVENTx() actually. We could use TRACE_EVENT directly, but given event and argument names and types have special meaning for a different subsystem, we'd like to pretend we have strongly-typed interface for these.
On Mon, Jul 22, 2013 at 11:44 AM, <caseq@chromium.org> wrote: > > https://codereview.chromium.**org/16848010/diff/49001/cc/** > trees/layer_tree_host.cc<https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.cc> > File cc/trees/layer_tree_host.cc (right): > > https://codereview.chromium.**org/16848010/diff/49001/cc/** > trees/layer_tree_host.cc#**newcode891<https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.cc#newcode891> > cc/trees/layer_tree_host.cc:**891: > devtools_instrumentation::**ScopedLayerTreeTask > On 2013/07/22 15:06:51, danakj wrote: > >> Just wondering: Does devtools have the ability to use our trace events >> > as well > >> for this kind of purpose? >> > > Yes. > > > If so, would it make sense to use a TRACE_EVENT for >> these instead? >> > > devtools_instrumentation::**ScopedFooTask classes are wrappers for > TRACE_EVENTx() actually. We could use TRACE_EVENT directly, but given > event and argument names and types have special meaning for a different > subsystem, we'd like to pretend we have strongly-typed interface for > these. > Ah, I see, that sounds great. Thanks for the explanation. > > https://codereview.chromium.**org/16848010/<https://codereview.chromium.org/1... >
https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:107: if (s_next_tree_id <= 0) what is this for? are you expecting to allocate more than 2 billion layer tree instances in the lifetime of one process? https://codereview.chromium.org/16848010/diff/49001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/16848010/diff/49001/content/renderer/render_v... content/renderer/render_view_impl.cc:943: webview()->devToolsAgent()->setLayerTreeId(rwc->GetLayerTreeId()); hmm, why is this grabbing the devtools agent via the webview? is it different from the devtools_agent_ parameter allocated on the previous line?
https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.cc File cc/trees/layer_tree_host.cc (right): https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.... cc/trees/layer_tree_host.cc:107: if (s_next_tree_id <= 0) On 2013/07/22 18:01:02, jamesr wrote: > what is this for? are you expecting to allocate more than 2 billion layer tree > instances in the lifetime of one process? This was mindlessly copied from layer.cc per Nat's suggestion above. I do wonder on why we keep ids signed and avoid negatives as well. I'm happy to drop the overflow check. Perhaps also make them unsigned? https://codereview.chromium.org/16848010/diff/49001/content/renderer/render_v... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/16848010/diff/49001/content/renderer/render_v... content/renderer/render_view_impl.cc:943: webview()->devToolsAgent()->setLayerTreeId(rwc->GetLayerTreeId()); On 2013/07/22 18:01:02, jamesr wrote: > hmm, why is this grabbing the devtools agent via the webview? is it different > from the devtools_agent_ parameter allocated on the previous line? Yes, it's different: webview() returns WebKit::WebView, so its devToolsAgent() returns WebKit::WebDevToolsAgent, not the chromium-side DevToolsAgent. Another way to say that would be devtools_agent_->GetWebAgent() (but that would expand to webview()->devToolsAgent() plus null checks).
On 2013/07/22 18:56:46, caseq wrote: > https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.cc > File cc/trees/layer_tree_host.cc (right): > > https://codereview.chromium.org/16848010/diff/49001/cc/trees/layer_tree_host.... > cc/trees/layer_tree_host.cc:107: if (s_next_tree_id <= 0) > On 2013/07/22 18:01:02, jamesr wrote: > > what is this for? are you expecting to allocate more than 2 billion layer tree > > instances in the lifetime of one process? > > This was mindlessly copied from layer.cc per Nat's suggestion above. I do wonder > on why we keep ids signed and avoid negatives as well. I'm happy to drop the > overflow check. Perhaps also make them unsigned? Overflow is not a practical concern here and int is just fine. We can burn through cc::Layers orders of magnitude faster than LayerTreeHosts, although the overflow check there is a little iffy. > > https://codereview.chromium.org/16848010/diff/49001/content/renderer/render_v... > File content/renderer/render_view_impl.cc (right): > > https://codereview.chromium.org/16848010/diff/49001/content/renderer/render_v... > content/renderer/render_view_impl.cc:943: > webview()->devToolsAgent()->setLayerTreeId(rwc->GetLayerTreeId()); > On 2013/07/22 18:01:02, jamesr wrote: > > hmm, why is this grabbing the devtools agent via the webview? is it different > > from the devtools_agent_ parameter allocated on the previous line? > > Yes, it's different: webview() returns WebKit::WebView, so its devToolsAgent() > returns WebKit::WebDevToolsAgent, not the chromium-side DevToolsAgent. Another > way to say that would be > devtools_agent_->GetWebAgent() (but that would expand to > webview()->devToolsAgent() plus null checks). Ah, I see - devtools_agent_ is not a WebDevToolsAgent. lgtm
Message was sent while issue was closed.
Committed patchset #7 manually as r213090 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
