1367576 - nsNativeThemeGTK should cache results of GetWidgetPadding/GetWidgetBorder

Status: RESOLVED FIXED

(Core :: Widget: Gtk, enhancement, P2)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsNativeThemeGTK should cache the results of GetWidgetPadding and GetWidgetBorder (and clear its cache when its ThemeChanged method is called).  These are somewhat expensive, and since they're used in layout (not just painting) can be called on very large numbers of elements, as in, e.g., bug 1118086 comment 38.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This will be easier once bug 1367577 is done.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I have patches for this, and they make pretty close to the expected performance improvement in opening the select in attachment 8833705 [details] in bug 1118086, which is a select with 10000 options.  Doing measuring using the Gecko Profiler, three times each:

without patch:
  5221ms pause in parent process, with 2392ms in nsNativeThemeGTK
  5001ms pause in parent process, with 2148ms in nsNativeThemeGTK
  5079ms pause in parent process, with 2111ms in nsNativeThemeGTK

with patch:
  3496ms pause in parent process, with 739ms in nsNativeThemeGTK 
  3547ms pause in parent process, with 764ms in nsNativeThemeGTK
  3524ms pause in parent process, with 778ms in nsNativeThemeGTK

Over 90% of the remaining time in nsNativeThemeGTK is spent inside nsMenuFrame::GetParentMenuListType.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(I should follow up and see if similar optimizations would be valuable on other platforms!  I think Windows wasn't nearly as bad, though.)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Refactor to allow for caching of some gtk widget padding/border results

This refactors the two nearly-identical callsites into a method so that
I can do caching in that method in the next patch.

Note that there was a slight difference between them in that the
aWidgetFlags parameter to GetGtkWidgetAndState was only passed from one
callsite.  However, given that the aState parameter is null, this
doesn't cause any behavior differences.  (Some controls in
GetGtkWidgetAndState null-check aWidgetFlags and some don't!)

Note also that this makes it always assign a result (often zero).  This
is fine for both callsites; GetWidgetPadding previously assigned zero
right before the call, and GetWidgetBorder did so at the start of the
function (and wasn't modified in between, since it was immediately
before the switch that the modified code is a case in).

MozReview-Commit-ID: IKurwry3UTi

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Cache results of getting GTK widget borders

See comments in the header file.

This also clears out mSafeWidgetStates in ThemeChanged since that seems
like a good thing to do, and marks nsNativeThemeGTK as final to avoid
the cost of the virtual function call to ThemeChanged in the
constructor.

MozReview-Commit-ID: 5Zne4eGbGlh

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Shrink existing caches to the size that's actually needed

Now that, thanks to bug 1367577, we have the theme constants in an enum,
we can make these arrays smaller rather than assuming that the constants
might use any valid uint8_t value.

MozReview-Commit-ID: A6GjTarVurc

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Make IsWidgetStateSafe not truncate the bits that it needs to test

This fix a mistake that goes back to the original code from bug 174585
(gecko-dev 9611b23530704402a714fa39cb433a01dca0bb6e, 2005-08-20).

(This makes me wonder how important the code is in the first place if it
didn't work correctly.)

MozReview-Commit-ID: B6q0o5n5hDw

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cba4f9e53cdbc50c129fd43c07e96bc62fca512&group_state=expanded

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Though I realize my comment about marking the class final isn't true, since the code I'm changing is in the constructor!)

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 8872068 [details] [diff] [review]
Make IsWidgetStateSafe not truncate the bits that it needs to test

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #7)
> (This makes me wonder how important the code is in the first place if it
> didn't work correctly.)

I guess it worked by disabling several widget types if one failed.

But it is not important now.  gdk_error_trap_pop() existed for catching default-fatal X protocol errors in GTK2 theme drawing.  GTK3 theme drawing no longer uses X surfaces, and so this is irrelevant now.

Comment 11

[Karl Tomlinson (:karlt)]

Thanks, David.  Are you able to make your profile data available to me, please?

The border values are more or less cached by GtkStyleContext objects also, and so I'd like to check that these caches are not being unnecessarily invalidated somehow.
It's hard to tell from the profile in https://bugzilla.mozilla.org/show_bug.cgi?id=1118086#c27 because it looks like there are only dynamic symbols for libgtk-3.so.

Retrieving from GtkStyleContext uses virtual function calls and there are conversions involved and so it is quite believable that an extra cache is beneficial, just because GtkStyleContext is too slow.

Comment 12

[Karl Tomlinson (:karlt)]

Comment on attachment 8872066 [details] [diff] [review]
Cache results of getting GTK widget borders

>+    uint8_t cacheIndex = gtkWidgetType / 8;
>+    uint8_t cacheBit = uint8_t(1) << (gtkWidgetType % 8);

No value in the uint8_t cast, as integral promotion will convert back to int
anyway (unless you are catering for systems were int has less precision that
uint8_t).

>+    if (mBorderCacheValid[cacheIndex] & cacheBit) {
>+      *aResult = mBorderCache[gtkWidgetType];
>+    } else {
>+      moz_gtk_get_widget_border(gtkWidgetType, &aResult->left, &aResult->top,
>+                                &aResult->right, &aResult->bottom, aDirection,
>+                                IsFrameContentNodeInNamespace(aFrame, kNameSpaceID_XHTML));

The moz_gtk_get_widget_border() call involves two parameters, direction and
inhtml, but these are not considered in the cache index.

MOZ_GTK_DROPDOWN is the only widget that depends on |direction|, so perhaps
this can be special-cased to skip the cache.  Not sure whether you want to do
that here, or in the caller so that the GtkTextDirection parameter is not
required.

|inhtml| doesn't cause problems, because it is not used, but please remove the
IsFrameContentNodeInNamespace() call to clarify.  Ideally the parameter can be
removed too as gtk2drawing.c is unused (bug 1278282).

Comment 13

[Karl Tomlinson (:karlt)]

Comment on attachment 8872065 [details] [diff] [review]
Refactor to allow for caching of some gtk widget padding/border results

Thanks for the detailed comments, David.

This is probably good, but I'll just remove the review request for now, until its clear how the direction is available for the dropdown, and I'd like to see what GTK is doing.
Let me know if you don't have that profiling data, in which case I'll do some profiling.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Here's a profile, which I think should show up already filtered on nsNativeThemeGTK: https://perfht.ml/2rqFWKo

Comment 15

[Karl Tomlinson (:karlt)]

Thanks.  I couldn't tell for certain from that profile, as apparently only dynamic symbols are found for libgtk, but the similar numbers of samples for each of GTK margin, border, and padding suggests that invalidation of GTK's cache is not happening before reading the first of those.

I confirmed with a debugger that GTK's cache is not being invalidated during layout, and so another cache is apparently the only way to make GetWidgetBorder() faster.  The approach here looks the appropriate solution, thanks, assuming adjustment to keep the current behaviour for MOZ_GTK_DROPDOWN.

I counted 120,000 calls to moz_gtk_get_widget_border() for the 10,000 menu items.  That implies that a bigger gain is possible with cached borders on each element/frame/box.  However, even with that, a cache for GetWidgetBorder()/moz_gtk_get_widget_border() would still provide significant benefit where there are many elements.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Remove unused ishtml parameter to moz_gtk_get_widget_border

This was needed for the (now-unused) GTK2 version of the code.

MozReview-Commit-ID: GocgC4OZ76p

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Refactor to allow for caching of some gtk widget padding/border results

This refactors the two nearly-identical callsites into a method so that
I can do caching in that method in the next patch.

Note that there was a slight difference between them in that the
aWidgetFlags parameter to GetGtkWidgetAndState was only passed from one
callsite.  However, given that the aState parameter is null, this
doesn't cause any behavior differences.  (Some controls in
GetGtkWidgetAndState null-check aWidgetFlags and some don't!)

Note also that this makes it always assign a result (often zero).  This
is fine for both callsites; GetWidgetPadding previously assigned zero
right before the call, and GetWidgetBorder did so at the start of the
function (and wasn't modified in between, since it was immediately
before the switch that the modified code is a case in).

MozReview-Commit-ID: IKurwry3UTi

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Cache results of getting GTK widget borders

See comments in the header file.

This also clears out mSafeWidgetStates in ThemeChanged since that seems
like a good thing to do, and marks nsNativeThemeGTK as final.

MozReview-Commit-ID: 5Zne4eGbGlh

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=70928c35847f3c007b9addb61e91e82ee1fc2d7a&group_state=expanded

Comment 20

[Karl Tomlinson (:karlt)]

Comment on attachment 8875009 [details] [diff] [review]
Cache results of getting GTK widget borders

Thanks!

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05cb421de1c392e23da293e03dd2d28a3d65457d
Bug 1367576 - Remove unused ishtml parameter to moz_gtk_get_widget_border.  r=karlt

https://hg.mozilla.org/integration/mozilla-inbound/rev/db68e1963b3974821cc065440f1b693a82721619
Bug 1367576 - Refactor to allow for caching of some gtk widget padding/border results.  r=karlt

https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e44a1cb4505c51d398db387c4c1d5c8afc462c
Bug 1367576 - Cache results of getting GTK widget borders.  r=karlt

https://hg.mozilla.org/integration/mozilla-inbound/rev/33997d929fda5555b857ce1aa0f523c99bfc0434
Bug 1367576 - Shrink existing caches to the size that's actually needed.  r=karlt

https://hg.mozilla.org/integration/mozilla-inbound/rev/143dcd21c6f0e94eb4c7cb7242061c7ef411b210
Bug 1367576 - Make IsWidgetStateSafe not truncate the bits that it needs to test.  r=karlt

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/05cb421de1c3
https://hg.mozilla.org/mozilla-central/rev/db68e1963b39
https://hg.mozilla.org/mozilla-central/rev/b8e44a1cb450
https://hg.mozilla.org/mozilla-central/rev/33997d929fda
https://hg.mozilla.org/mozilla-central/rev/143dcd21c6f0

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

I see a perf improvement from this patch:
== Change summary for alert #7093 (as of June 07 2017 05:28 UTC) ==

Improvements:

  2%  cart summary linux64 opt e10s     25.31 -> 24.75

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7093

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Good.  That's not too surprising.