Bug 162673 - [GTK] Cache RenderThemeGadget instances for rendering scrollbars for GTK+ 3.20+
Summary: [GTK] Cache RenderThemeGadget instances for rendering scrollbars for GTK+ 3.20+
Status: NEW
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: 2016-12-22 12:46 PST (History)
7 users (show)

See Also:


Attachments
Patch (80.55 KB, patch)
2016-09-28 07:14 PDT, Andre Moreira Magalhaes
andrunko: review?
Details | Formatted Diff | Diff

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.