Closed Bug 1373079 Opened 7 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
Widget: Win32
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: MatsPalmgren_bugz, Assigned: alexical)

References

(Blocks 2 open bugs,
URL
)

Details

(Keywords: perf, Whiteboard: [tpi:+])

Attachments

(3 files)

Bug 1373079 - (1) Cache GetWidgetBorder
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 1373079 - (2) Cache GetMinimumWidgetSize
59 bytes, text/x-review-board-request
jimm
: review+
Details
Bug 1373079 - (3) Special-case min-size cache for buttons
59 bytes, text/x-review-board-request
jimm
: review+
Details 
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).
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: qf-bugs-upforgrabs
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: Speedometer_V2
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)
Priority: P2 → P1
Whiteboard: [qf:p1] → [qf:p1][tpi:+]
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)
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.
(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 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 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+
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)
Clearing the needinfo since it's redundant now with the r?jimm request.
Flags: needinfo?(jmathies)
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+
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
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
Closed: 7 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.
No longer blocks: 1380629
Depends on: 1380629
Performance Impact: --- → P1
Whiteboard: [qf:p1][tpi:+] → [tpi:+]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: