1373079 - nsNativeThemeWin::GetWidgetBorder and nsNativeThemeWin::GetMinimumWidgetSize are too slow

Status: RESOLVED FIXED

(Core :: Widget: Win32, enhancement, P1)

Description

[Mats Palmgren (inactive)]

These methods have a combined ~8% in the performance log for the Netflix
use case reported in bug 1373058.  This seems way too much.

Would it be possible to cache the result for each widget type?
(and reset the cache on a theme change)

I think we do that on other platforms (e.g. GTK).

Comment 1

[Jean Gong :jgong]

Jim,  Can you provide your input on the LOE and feasibility of putting in a fix on this for fx57 to beneifit QF performance.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Here's a profile showing the issue: https://perfht.ml/2t6Vaon

Comment 3

[Jim Mathies [:jimm]]

Yes I think we can do some caching here.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Jean Gong from comment #1)
> Jim,  Can you provide your input on the LOE and feasibility of putting in a
> fix on this for fx57 to beneifit QF performance.

Doesn't look like the LOE would be high, maybe a day or two tops.

Comment 5

[Andrew Overholt [:overholt]]

Let's just fix this :)

Comment 6

[Kan-Ru Chen [:kanru] (UTC+9)]

Adding to qf-bugs-upforgrabs assuming we only need to add caches to these functions.

Comment 7

[Mats Palmgren (inactive)]

Fwiw, I noticed that there is a lot of checkboxes in the Speedometer test,
so fixing this bug *might* help improving that on Windows.

Comment 8

[Alex Thayer [:alexical] (she/her)]

I'd like to take this if it's up for grabs.

Comment 9

[Jim Mathies [:jimm]]

(In reply to Doug Thayer [:dthayer] from comment #8)
> I'd like to take this if it's up for grabs.

all yours, you can flag me for review.

Comment 10

[Alex Thayer [:alexical] (she/her)]

I'm noticing that in the profile almost all of the time for nsNativeThemeWin::GetMinimumWidgetSize is spent in GetDC and ReleaseDC. Do you know why we even need to get the screen's HDC for this? The MSDN documentation for this parameter at least for GetThemePartSize just says that it's an "HDC to select fonts into". In my header file it's "HDC to select font into & measure against", but in either case I can't get anything to behave differently when I specify null for this. Is there something I'm missing?

Comment 11

[Jim Mathies [:jimm]]

(In reply to Doug Thayer [:dthayer] from comment #10)
> I'm noticing that in the profile almost all of the time for
> nsNativeThemeWin::GetMinimumWidgetSize is spent in GetDC and ReleaseDC. Do
> you know why we even need to get the screen's HDC for this? The MSDN
> documentation for this parameter at least for GetThemePartSize just says
> that it's an "HDC to select fonts into". In my header file it's "HDC to
> select font into & measure against", but in either case I can't get anything
> to behave differently when I specify null for this. Is there something I'm
> missing?

No I don't think so. My guess is that this call will fail in some cases where the internal api needs an HDC for font calculations for certain parts and states, and succeed when it doesn't. Trying to predict which parts and states need the hdc might be a bit buggy but we could work through it in Nightly. In this particular case we only have three queries below the GetHDC calls, none of which appear to involve fonts.

For an experiment, I'd suggest adding a wrapper for GetThemePartSize that debug assert on failure, pass null in all cases there, and push that to try for a full windows browser chrome test run, see what shows up!

Comment 12

[Mats Palmgren (inactive)]

It might also depend on which native theme the user is using?

Anyway, if we only do it once (per widget type) and cache the results,
then a GetDC/ReleaseDC isn't a problem.

Comment 13

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1373079 - (1) Cache GetWidgetBorder

Both GetWidgetBorder and GetMinimumWidgetSize are showing up
in some profiles (see bug for more details.) This is the first
patch in a series of patches which cache the results of these
functions.

Because aWidgetType can map to multiple theme parts, in order to
cover as much as possible with our cache we decided to cache
based off of the theme class and the theme part, which are derived
from the aWidgetType and misc. other state. (Assumption: the
widget border and minimum widget size should not changed based on
the theme "state" (the value that accompanies the "part".))

The total cache size for these, if we use plain arrays, is 18KB.
We could reduce this by some amount by using a sparse dynamically
sized cache or by just using aWidgetType and discarding the
overloaded values, which are few. I don't have a great intuition
for how much we care about saving a few KB, or how much time this
could cause us to lose on L1 and L2 cache misses. Accordingly it
might be more optimal to go with something else, and I am open to
criticism/suggestions.

Review commit: https://reviewboard.mozilla.org/r/151870/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151870/

Comment 14

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1373079 - (2) Cache GetMinimumWidgetSize

See commit (1) for more detail about the bug.

This patch caches the expensive parts of GetMinimumWidgetSize,
which are when we call GetDC and ReleaseDC. The exits before this
cached section don't have their results cached partly because
they don't seem to show up in profiles, and partly because we
don't necessarily have a theme part at that point, which means
we would need to have a more complicated caching scheme directly
involving the aWidgetType.

Review commit: https://reviewboard.mozilla.org/r/151872/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/151872/

Comment 15

[Alex Thayer [:alexical] (she/her)]

(In reply to Jim Mathies [:jimm] from comment #11)
> For an experiment, I'd suggest adding a wrapper for GetThemePartSize that
> debug assert on failure, pass null in all cases there, and push that to try
> for a full windows browser chrome test run, see what shows up!

I haven't done this yet since, agreeing Mats's comment, the GetDC/ReleaseDC shouldn't be an issue once cached. However I'm open to exploring getting rid of it anyway if you think that's a good idea.

In any case, my plan for testing this is to add in a patch that computes both the cached and the uncached value and asserts that they're identical, and then run that through a browser chrome test run similar to your above suggestion. Before doing this though I was just hoping to get an overall review of the patch to make sure it's not totally off target. Does this sound reasonable? Do you have any other ideas for ways to test this, Jim (or anyone else)?

Comment 16

[Jim Mathies [:jimm]]

Comment on attachment 8880576 [details]
Bug 1373079 - (1) Cache GetWidgetBorder

https://reviewboard.mozilla.org/r/151870/#review157796

::: commit-message-5456e:13
(Diff revision 1)
> +Because aWidgetType can map to multiple theme parts, in order to
> +cover as much as possible with our cache we decided to cache
> +based off of the theme class and the theme part, which are derived
> +from the aWidgetType and misc. other state. (Assumption: the
> +widget border and minimum widget size should not changed based on
> +the theme "state" (the value that accompanies the "part".))

I wonder if border migth be affected by disabled state. hard to say, I'd say lets land this and keep an eye out for incoming polish bugs.

::: commit-message-5456e:16
(Diff revision 1)
> +from the aWidgetType and misc. other state. (Assumption: the
> +widget border and minimum widget size should not changed based on
> +the theme "state" (the value that accompanies the "part".))
> +
> +The total cache size for these, if we use plain arrays, is 18KB.
> +We could reduce this by some amount by using a sparse dynamically

this code executes in each content process currently, but we'll remote it to a single process eventually. I think we can eat 18K * 5 or 18K * 9 (future) for now.

::: widget/windows/nsNativeThemeWin.h:127
(Diff revision 1)
>    void DrawThemedProgressMeter(nsIFrame* aFrame, int aWidgetType,
>                                 HANDLE aTheme, HDC aHdc,
>                                 int aPart, int aState,
>                                 RECT* aWidgetRect, RECT* aClipRect);
>  
> +  nsresult GetCachedWidgetBorder(nsIFrame* aFrame, nsUXThemeClass themeClass,

aThemeClass

::: widget/windows/nsNativeThemeWin.cpp:613
(Diff revision 1)
> +  outerRect.right = outerRect.bottom = 200;
> +  RECT contentRect(outerRect);
> +  HRESULT res = GetThemeBackgroundContentRect(theme, nullptr, aPart, aState, &outerRect, &contentRect);
> +
> +  if (FAILED(res))
> +    return NS_ERROR_FAILURE;

nit - paren around this

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 8880577 [details]
Bug 1373079 - (2) Cache GetMinimumWidgetSize

https://reviewboard.mozilla.org/r/151872/#review157798

::: widget/windows/nsNativeThemeWin.h:131
(Diff revision 1)
>  
>    nsresult GetCachedWidgetBorder(nsIFrame* aFrame, nsUXThemeClass themeClass,
>                                   uint8_t aWidgetType, int32_t aPart, int32_t aState,
>                                   nsIntMargin* aResult);
>  
> +  nsresult GetCachedMinimumWidgetSize(nsIFrame* aFrame, HANDLE theme, nsUXThemeClass themeClass,

nit - touch up parameters names

::: widget/windows/nsNativeThemeWin.cpp:628
(Diff revision 1)
>    mBorderCache[cacheIndex] = *aResult;
>  
>    return NS_OK;
>  }
>  
> +nsresult nsNativeThemeWin::GetCachedMinimumWidgetSize(nsIFrame * aFrame, HANDLE theme,

nit - ditto

::: widget/windows/nsNativeThemeWin.cpp:644
(Diff revision 1)
> +    *aResult = mMinimumWidgetSizeCache[cacheIndex];
> +    return NS_OK;
> +  }
> +
> +  HDC hdc = ::GetDC(NULL);
> +  if (!hdc)

nit - paren me

Comment 18

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8880576 [details]
Bug 1373079 - (1) Cache GetWidgetBorder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151870/diff/1-2/

Comment 19

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8880577 [details]
Bug 1373079 - (2) Cache GetMinimumWidgetSize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151872/diff/1-2/

Comment 20

[Alex Thayer [:alexical] (she/her)]

Hey Jim, I ran this through try failing on any differences between cached and uncached results, and there's one case I missed where aSizeReq can vary for a particular aThemeClass and aPart combination, so I special-cased it as follows:

  int32_t cachePart = aPart;
  if (aWidgetType == NS_THEME_BUTTON && aSizeReq == TS_MIN) {
    // In practice, NS_THEME_BUTTON is the only widget type which has an aSizeReq
    // that varies for us, and it can only be TS_MIN or TS_TRUE. Just stuff that
    // extra bit into the aPart part of the cache, since BP_Count is well below
    // THEME_PART_DISTINCT_VALUE_COUNT anyway.
    cachePart = BP_Count;
  }

  MOZ_ASSERT(aPart < THEME_PART_DISTINCT_VALUE_COUNT);
  int32_t cacheIndex = aThemeClass * THEME_PART_DISTINCT_VALUE_COUNT + cachePart;
  int32_t cacheBitIndex = cacheIndex / 8;
  uint8_t cacheBit = 1u << (cacheIndex % 8);

(BP_Count was added to nsUXThemeConstants.h)

Does this seem all right to you? Other than that everything is all green.

Comment 21

[Alex Thayer [:alexical] (she/her)]

Attachment: Bug 1373079 - (3) Special-case min-size cache for buttons

Testing for cache differences via assertions on try revealed that
we need to special-case buttons in our cache, since they can have
two different values for aSizeReq. Visual inspection of the code
reveals that this is the only aWidgetType value that has this
problem. I'm not sure how future-proof we want to try to be with
this. If anything else starts varying in this parameter then we
might unwittingly introduce caching problems, but I don't know
how likely that is.

Review commit: https://reviewboard.mozilla.org/r/155266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155266/

Comment 22

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8880576 [details]
Bug 1373079 - (1) Cache GetWidgetBorder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151870/diff/2-3/

Comment 23

[Alex Thayer [:alexical] (she/her)]

Comment on attachment 8880577 [details]
Bug 1373079 - (2) Cache GetMinimumWidgetSize

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/151872/diff/2-3/

Comment 24

[Alex Thayer [:alexical] (she/her)]

Clearing the needinfo since it's redundant now with the r?jimm request.

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 8884338 [details]
Bug 1373079 - (3) Special-case min-size cache for buttons

https://reviewboard.mozilla.org/r/155266/#review160966

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4dd0c01a271c
(1) Cache GetWidgetBorder r=jimm
https://hg.mozilla.org/integration/autoland/rev/25cd2441337e
(2) Cache GetMinimumWidgetSize r=jimm
https://hg.mozilla.org/integration/autoland/rev/31450261d6f7
(3) Special-case min-size cache for buttons r=jimm

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4dd0c01a271c
https://hg.mozilla.org/mozilla-central/rev/25cd2441337e
https://hg.mozilla.org/mozilla-central/rev/31450261d6f7

Comment 28

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

I see a perf improvement from this:
== Change summary for alert #7836 (as of July 11 2017 00:15 UTC) ==

Improvements:

  6%  a11yr summary windows10-64 opt e10s     635.05 -> 594.73

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

Comment 29

[Bill Gianopoulos [:WG9s]]

This breaks the Browser UI for Windows 7 users who use the Windows classic desktop theme.  See bug 1380629.