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

RESOLVED FIXED in Firefox 56

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: dthayer)

Tracking

(Blocks: 3 bugs, {perf})

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [qf:p1][tpi:+], URL)

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
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

2 years ago
Jim,  Can you provide your input on the LOE and feasibility of putting in a fix on this for fx57 to beneifit QF performance.
Flags: needinfo?(jmathies)
Here's a profile showing the issue: https://perfht.ml/2t6Vaon
Yes I think we can do some caching here.
Flags: needinfo?(jmathies)
(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.
Let's just fix this :)
Flags: needinfo?(jmathies)
Whiteboard: [qf] → [qf:p1]
Adding to qf-bugs-upforgrabs assuming we only need to add caches to these functions.
Blocks: 1364015
(Reporter)

Comment 7

2 years ago
Fwiw, I noticed that there is a lot of checkboxes in the Speedometer test,
so fixing this bug *might* help improving that on Windows.
Blocks: 1339557
(Assignee)

Comment 8

2 years ago
I'd like to take this if it's up for grabs.
Assignee: nobody → dothayer
Status: NEW → ASSIGNED
(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.
Flags: needinfo?(jmathies)

Updated

2 years ago
Priority: P2 → P1
Whiteboard: [qf:p1] → [qf:p1][tpi:+]
(Assignee)

Comment 10

2 years ago
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?
Flags: needinfo?(jmathies)
(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!
Flags: needinfo?(jmathies)
(Reporter)

Comment 12

2 years ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 years ago
(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

2 years ago
mozreview-review
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
Attachment #8880576 - Flags: review?(jmathies) → review+

Comment 17

2 years ago
mozreview-review
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
Attachment #8880577 - Flags: review?(jmathies) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 20

2 years ago
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.
Flags: needinfo?(jmathies)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

2 years ago
Clearing the needinfo since it's redundant now with the r?jimm request.
Flags: needinfo?(jmathies)

Comment 25

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

https://reviewboard.mozilla.org/r/155266/#review160966
Attachment #8884338 - Flags: review?(jmathies) → review+

Comment 26

2 years ago
Pushed by dothayer@mozilla.com:
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4dd0c01a271c
https://hg.mozilla.org/mozilla-central/rev/25cd2441337e
https://hg.mozilla.org/mozilla-central/rev/31450261d6f7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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
This breaks the Browser UI for Windows 7 users who use the Windows classic desktop theme.  See bug 1380629.

Updated

2 years ago
No longer blocks: 1380629
Depends on: 1380629
You need to log in before you can comment on or make changes to this bug.