Bug 162673 - [GTK] Cache RenderThemeGadget hierarchies for rendering themed elements with GTK+ 3.20+
Summary: [GTK] Cache RenderThemeGadget hierarchies for rendering themed elements with ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Gtk (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andre Moreira Magalhaes
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-09-28 06:52 PDT by Andre Moreira Magalhaes
Modified: 2017-06-01 23:40 PDT (History)
8 users (show)

See Also:


Attachments
Patch (80.55 KB, patch)
2016-09-28 07:14 PDT, Andre Moreira Magalhaes
cgarcia: review-
Details | Formatted Diff | Diff
Patch (106.30 KB, patch)
2017-05-24 10:06 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Try to fix the build with older GTK+ (106.34 KB, patch)
2017-05-24 10:16 PDT, Carlos Garcia Campos
mcatanzaro: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (1002.81 KB, application/zip)
2017-05-24 11:45 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Moreira Magalhaes 2016-09-28 06:52:04 PDT
GTK+ styling machinery is currently taking non-trivial amounts of time to render scrollbars on simple pages. The problem is that the current code on ScrollbarThemeGtk creates instances of RenderThemeGadget like there’s no tomorrow :D, and each of those creates a GtkStyleContext and sets it up. Given that GtkStyleContext is designed to be cached we should take advantage of that.

Patch to follow.
Comment 1 Andre Moreira Magalhaes 2016-09-28 07:14:52 PDT
Created attachment 290082 [details]
Patch
Comment 2 Michael Catanzaro 2016-10-05 09:31:29 PDT
Carlos Garcia, do you want to review this? I can do it, but it's rewriting a lot of your code so I guess you might prefer to.
Comment 3 Carlos Garcia Campos 2016-10-05 23:16:22 PDT
Yes, it's in my todo, after a first glance my first thought was that we could try to make this more generic, so that it will work for RenderTheme as well, I guess a page with a lot of forms could be also affected. We used to cache the style contexts, but we removed that I don't remember why.
Comment 4 Michael Catanzaro 2016-10-06 06:29:17 PDT
(In reply to comment #3)
> Yes, it's in my todo, after a first glance my first thought was that we
> could try to make this more generic, so that it will work for RenderTheme as
> well, I guess a page with a lot of forms could be also affected. We used to
> cache the style contexts, but we removed that I don't remember why.

I removed it because it was broken at the time, and I failed to fix it because GtkStyleContext APIs never do what you think (e.g. save() and restore() add or remove style nodes :S) and removing the cache simplified the code quite a bit. I don't mind adding it back if it improves performance and isn't buggy. Ideally we would switch layout tests to use GTK+ 3.22 before doing so, though.
Comment 5 Michael Catanzaro 2016-10-06 06:38:01 PDT
It was in r193896. The commit message there says I intended to check the performance implications on the perf bot, but I never did and the bots are still running GTK+ 3.16 anyway. :)
Comment 6 Michael Catanzaro 2016-12-22 12:46:00 PST
(In reply to comment #3)
> Yes, it's in my todo, after a first glance my first thought was that we
> could try to make this more generic, so that it will work for RenderTheme as
> well, I guess a page with a lot of forms could be also affected. We used to
> cache the style contexts, but we removed that I don't remember why.

Hm, this bug still exists. We surely have bigger problems right now, but we don't want to forget about this.
Comment 7 Carlos Garcia Campos 2017-05-24 09:49:38 PDT
Comment on attachment 290082 [details]
Patch

I've been looking at this in detail. It's tricky, as usual with GTK+ theming. This approach doesn't really work, mainly because gtk_style_context_save/restore breaks the gadgets hierarchy when using GtkWidgetPath. This might not be a problem for some themes, or some widgets like scrollbars, but will break for sure themes that rely on inheritance when querying css properties, and styles using  information about siblings like the combobox. Adding/removing classes to the GtkStyleContext doesn't work when usingGtkWidgetPath either, classes needs to be present when building the GtkWidgetPath. So, we can't cache style contexts or gadgets, we need to cache hierarchies of gadgets with fixed style classes. Fortunately, setting the state on a GtkStyleContext does work, so we can cache more generic hierarchies and apply the state later. In the particular case of scrollbars we would cache VerticalScrollbarRight, VerticalScrollbarLeft, HorizontalScrollbar, VerticalScrollIndicatorRight, VerticalScrollIndicatorLeft and HorizontalScrollIndicator. In practice, we will only have 4 of those at the same time in the cache.
Comment 8 Carlos Garcia Campos 2017-05-24 10:06:11 PDT
Created attachment 311125 [details]
Patch
Comment 9 Carlos Garcia Campos 2017-05-24 10:16:15 PDT
Created attachment 311127 [details]
Try to fix the build with older GTK+
Comment 10 Build Bot 2017-05-24 11:45:08 PDT
Comment on attachment 311127 [details]
Try to fix the build with older GTK+

Attachment 311127 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3807939

New failing tests:
http/tests/xmlhttprequest/xmlhttprequest-50ms-download-dispatch.html
Comment 11 Build Bot 2017-05-24 11:45:10 PDT
Created attachment 311140 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.5
Comment 12 Michael Catanzaro 2017-05-31 19:38:53 PDT
Comment on attachment 311127 [details]
Try to fix the build with older GTK+

View in context: https://bugs.webkit.org/attachment.cgi?id=311127&action=review

I've restored to just skimming this, since I've had it open awaiting review for over a week now, and it looks fine. Thanks for working on this massive effort. Please don't backport it; you never know what odd themes could break.

> Source/WebCore/platform/gtk/RenderThemeWidget.cpp:176
> +    RenderThemeGadget::Info info = {
> +        .type = RenderThemeGadget::Type::Generic,
> +        .name = toggleType == Type::Check ? "checkbutton" : "radiobutton",
> +        .classList = { "text-button" }
> +    };

Let's not use nonstandard designated initializers (see bug #172667)
Comment 13 Carlos Garcia Campos 2017-06-01 23:40:48 PDT
Committed r217702: <http://trac.webkit.org/changeset/217702>