Closed Bug 1367576 Opened 2 years ago Closed 2 years ago

nsNativeThemeGTK should cache results of GetWidgetPadding/GetWidgetBorder

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: perf, Whiteboard: tpi:+)

Attachments

(5 files, 2 obsolete files)

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.
This will be easier once bug 1367577 is done.
Blocks: 1118086
Depends on: 1367577
Mentor: dbaron
Keywords: perf
Whiteboard: [lang=c++][good sixth bug]
Assignee: nobody → dbaron
Mentor: dbaron
Status: NEW → ASSIGNED
Whiteboard: [lang=c++][good sixth bug]
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.
(I should follow up and see if similar optimizations would be valuable on other platforms!  I think Windows wasn't nearly as bad, though.)
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
Attachment #8872065 - Flags: review?(karlt)
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
Attachment #8872066 - Flags: review?(karlt)
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
Attachment #8872067 - Flags: review?(karlt)
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
Attachment #8872068 - Flags: review?(karlt)
(Though I realize my comment about marking the class final isn't true, since the code I'm changing is in the constructor!)
Attachment #8872067 - Flags: review?(karlt) → review+
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.
Attachment #8872068 - Flags: review?(karlt) → review+
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.
Flags: needinfo?(dbaron)
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).
Attachment #8872066 - Flags: review?(karlt) → review-
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.
Attachment #8872065 - Flags: review?(karlt)
Here's a profile, which I think should show up already filtered on nsNativeThemeGTK: https://perfht.ml/2rqFWKo
Flags: needinfo?(dbaron)
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.
This was needed for the (now-unused) GTK2 version of the code.

MozReview-Commit-ID: GocgC4OZ76p
Attachment #8875006 - Flags: review?(karlt)
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
Attachment #8875007 - Flags: review?(karlt)
Attachment #8872065 - Attachment is obsolete: true
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
Attachment #8875009 - Flags: review?(karlt)
Attachment #8872066 - Attachment is obsolete: true
Attachment #8875006 - Flags: review?(karlt) → review+
Attachment #8875007 - Flags: review?(karlt) → review+
Comment on attachment 8875009 [details] [diff] [review]
Cache results of getting GTK widget borders

Thanks!
Attachment #8875009 - Flags: review?(karlt) → review+
Priority: -- → P2
Whiteboard: tpi:+
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
Good.  That's not too surprising.
You need to log in before you can comment on or make changes to this bug.