Bug 150392 – [GTK] Implement rendering frames timeline panel for GTK port

• Component: Web Inspector
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

The Web Inspector has now a nice view to show the rendering times when in accelerated compositing mode, but that doesn't work in GTK+. It's quite confusing for the users, because the panel is in the inspector but it is never updated. This is because we don't implement the instrumentation required. The mac implementation is based on RunLoopObserver, that only has an implementation based on CF. It's not currently possible to do the same with the glib main loop, so even if there's a proper platform abstraction of RunLoopObserver, we won't be able to provide a glib implementation. Maybe when persistent sources patch lands (see bug #138691), we can provide a way to monitor our sources. For now, what we can do is to use a custom persistent source for the AC rendering, instead of a GMainLoopSource and notify the inspector on every source dispatch, after and before actually dispatching the source callback. And it will improve the performance for the same price.

[Radar WebKit Bug Importer]

<rdar://problem/23200510>

[Radar WebKit Bug Importer]

<rdar://problem/23200512>

[Carlos Garcia Campos]

Created attachment 263677 [details]
Patch

[Blaze Burg]

Comment on attachment 263677 [details]
Patch

I don't think this will do the right thing when the debugger pauses and we use a nested run loop. Can you test it to see what happens?

I do think that it is a little weird that these events do not go through InspectorInstrumentation already. There is no event system in place to notify WK2 clients when specific domains have instrumentation enabled or disabled, so there's no good way to selectively add/remove RunLoopObservers only when necessary. Thus, it's put into TimelineAgent's start/stop code itself.

[Blaze Burg]

(In reply to comment #0)
> The mac implementation is based on RunLoopObserver, that only has
> an implementation based on CF. It's not currently possible to do the same
> with the glib main loop, so even if there's a proper platform abstraction of
> RunLoopObserver, we won't be able to provide a glib implementation. Maybe
> when persistent sources patch lands (see bug #138691), we can provide a way
> to monitor our sources.

If you could provide some rationale for why RunLoopObserver isn't a sufficient abstraction in <https://bugs.webkit.org/show_bug.cgi?id=142748>, that would be helpful in moving that discussion forward.

[Carlos Garcia Campos]

(In reply to comment #4)
> Comment on attachment 263677 [details]
> Patch
> 
> I don't think this will do the right thing when the debugger pauses and we
> use a nested run loop. Can you test it to see what happens?

Ah, right, I forgot about the debugger, I'll check it.

> I do think that it is a little weird that these events do not go through
> InspectorInstrumentation already. There is no event system in place to
> notify WK2 clients when specific domains have instrumentation enabled or
> disabled, so there's no good way to selectively add/remove RunLoopObservers
> only when necessary. Thus, it's put into TimelineAgent's start/stop code
> itself.

In our case we need to notify the inspector from WebKit2, because we can't monitor the run loop to observe the sources.

[Carlos Garcia Campos]

(In reply to comment #5)
> (In reply to comment #0)
> > The mac implementation is based on RunLoopObserver, that only has
> > an implementation based on CF. It's not currently possible to do the same
> > with the glib main loop, so even if there's a proper platform abstraction of
> > RunLoopObserver, we won't be able to provide a glib implementation. Maybe
> > when persistent sources patch lands (see bug #138691), we can provide a way
> > to monitor our sources.
> 
> If you could provide some rationale for why RunLoopObserver isn't a
> sufficient abstraction in <https://bugs.webkit.org/show_bug.cgi?id=142748>,
> that would be helpful in moving that discussion forward.

Yes, the glib main loop doesn't provide any way to know its status, and there isn't any GMainLoopObserver in glib either. We could implement something like that in glib, though, but in the meantime we need a different way to implement at least the rendering frames panel. What we can do with the current glib main loop, is to use custom sources, that we can dispatch. That way we can do whatever before and after the source is actually dispatched, which is what I'm doing in this patch. We plan to change some glib sources used in WebKit with custom sources like the one I'm using in this patch, but in a more generic way. Once we have that we could implement a RunLoopObserver, but only to monitor our own custom sources.

[Blaze Burg]

Comment on attachment 263677 [details]
Patch

Matt, can you help review this patch? It would be great to use only one code path to deliver will/didRenderFrame. Is it possible we can route runLoopObserver's callbacks through InspectorInstrumentation and have it live outside of TimelineAgent? This would make me a lot more confident about this working for nested run loops. Or, if that's not going to work, we might have to think up a different design.

[Carlos Garcia Campos]

I've tried to test the debugger, but for some reason the inspector web process crashes here just by clicking on the debugger tab. I've been working on using a persistent source in the RunLoop implementation that would allow us to implement RunLoopObserver to monitor tasks run in a RunLoop, not any source in the GMainLoop, though, but it could be enough for us. See patch attached to bug #138691.

[Blaze Burg]

(In reply to comment #9)
> I've tried to test the debugger, but for some reason the inspector web
> process crashes here just by clicking on the debugger tab.

If it's unrelated to this patch, definitely file a bug! Crashes are bad.

I've been working
> on using a persistent source in the RunLoop implementation that would allow
> us to implement RunLoopObserver to monitor tasks run in a RunLoop, not any
> source in the GMainLoop, though, but it could be enough for us. See patch
> attached to bug #138691.

Sounds good. If it turns out to be too much work, we can figure out a way to make the frontend hide the rendering frames view when the backend doesn't support it.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> > I've tried to test the debugger, but for some reason the inspector web
> > process crashes here just by clicking on the debugger tab.
> 
> If it's unrelated to this patch, definitely file a bug! Crashes are bad.

Yes, it's unrelated, I was waiting to check if it still happens in current trunk.

> I've been working
> > on using a persistent source in the RunLoop implementation that would allow
> > us to implement RunLoopObserver to monitor tasks run in a RunLoop, not any
> > source in the GMainLoop, though, but it could be enough for us. See patch
> > attached to bug #138691.
> 
> Sounds good. If it turns out to be too much work, we can figure out a way to
> make the frontend hide the rendering frames view when the backend doesn't
> support it.

Yes, well, but this is quite useful feature we want to support :-)

[Matt Baker]

(In reply to comment #8)
> Comment on attachment 263677 [details]
> Patch
> 
> Matt, can you help review this patch? It would be great to use only one code
> path to deliver will/didRenderFrame. Is it possible we can route
> runLoopObserver's callbacks through InspectorInstrumentation and have it
> live outside of TimelineAgent? This would make me a lot more confident about
> this working for nested run loops. Or, if that's not going to work, we might
> have to think up a different design.

I'll need to give this some thought, and look over the glib main event loop docs (https://developer.gnome.org/glib/stable/glib-The-Main-Event-Loop.html). I have enough experience with the Win32 message pump to have an idea of how to implement this for the Windows port, but have no experience with glib. According to Carlos it seems there are some obstacles (hopefully not insurmountable) to instrumenting glib's event loop.

[Blaze Burg]

Carlos, will this patch be affected by recent work on GTK run loop? If so, we should reset r? until you have amended the patch.

[Carlos Garcia Campos]

Comment on attachment 263677 [details]
Patch

Yes, let's clear flags for now.

[Carlos Garcia Campos]

We have switched all our sources to use RunLoop, so we could now implement a RunLoopObserver for a RunLoop. The problem is how to identify the sources to filter the notifications, because RunLoop works with std::function and RunLoop::Timer with function pointer. For now we could limit it to monitor RunLoop::Timers only. We could set a unique id to RunLoop::Timer and add API to get it.

[Carlos Garcia Campos]

Many things have changed since the first time I tried to implement this for GTK port. Now we always use the threaded compositor for accelerated rendering, which means that frames are rendered in a secondary thread. That complicates things a bit, because we need to notify the inspector in the main thread. My initial patch  notified about frame start right before the contents were rendered into the GL context and the frame end after the swap buffers. But I'm not sure that's actually what we want here. I don't know how Core Animation works, so could someone explain when is begin/end frame and will/did composite expected to happen?

[Carlos Garcia Campos]

hmm, I've just read https://webkit.org/blog/3996/introducing-the-rendering-frames-timeline/ So, every frame is actually a main run loop iteration? not necessarily a loop iteration in which the frames are rendered? I guess other run loop sources are just discarded because the record has no children (no style recalc, no paint, etc.). If that's the case, then I think we can implement a RunLoopObserver for WTF::RunLoop.

[Carlos Garcia Campos]

Created attachment 406841 [details]
Patch

This patch follows the run loop approach. We can't monitor the run loop because GLib doesn't have API for that, but we can at least monitor our sources, which seems to be enough to get quite useful information. It's not exactly a frame per loop cycle in some cases, because here we are monitoring the sources, but in most of the cases there's only one source per loop iteration.

[Carlos Garcia Campos]

Btw, the patch uses glib event loop ifdefs. I think it should be possible to use the same API for cocoa, probably moving the run loop observer to WTF and using that in RunLoop. That could be done in a follow up as part of bug #142748.

[Carlos Garcia Campos]

Brian?

[Blaze Burg]

Comment on attachment 406841 [details]
Patch

This change looks like it's in the right direction. Unfortunately the engineers who wrote and reviewed this code are no longer available, so I'd need to do more research on existing vs desired behavior to know if your proposed refactoring will work out on Cocoa platforms.

[Carlos Garcia Campos]

Committed r266103: <https://trac.webkit.org/changeset/266103>