894099 - Use WindowDraggingUtils.jsm for dragging the window on XP because the hit test is slow

Status: RESOLVED WONTFIX

(Core :: Widget: Win32, defect)

Description

[Matthew N. [:MattN]]

Markus had a patch to improve the performance of nsWindow::ClientMarginHitTestPoint. Filing this so we don't lose track of it.

Try push with the broken WIP patch: https://tbpl.mozilla.org/?tree=Try&rev=6ff835275e8c

Comment 1

[Matthew N. [:MattN]]

Updated try push after the Try server reset: https://tbpl.mozilla.org/?tree=Try&rev=d341ee4efa6d

Comment 2

[:Gijs (he/him)]

Attachment: Patch MattN ran against try

So this is what Matt ran against try earlier, with good results. Unfortunately we probably can't Just Do that, because on Win7/Win8 we need to integrate with Aero Snap. Matt said we can check the Windows version with WinUtils::GetWindowsVersion to work that out, and not run that bit of code on XP. Not sure about Vista.

Comment 3

[:Gijs (he/him)]

Attachment: 894099-optimize-client-hittest

From talking to Matt on IRC this morning, I think this is what we're aiming for. Felipe, can you sanity check my approach?

I've pushed this to try:

https://tbpl.mozilla.org/?tree=Try&rev=119748aeea3b

(baseline: https://tbpl.mozilla.org/?tree=Try&rev=e9bb3558a0f8 )

Comment 4

[Markus Stange [:mstange]]

I've fixed my patch to actually send the hittest event with correct coordinates (the last patch was missing a conversion from screen coordinates to window coordinates), and the try results are somewhere between the two pushes in comment 3:
https://tbpl.mozilla.org/?tree=Try&rev=084e069997f8

Comment 5

[:Gijs (he/him)]

(In reply to Markus Stange [:mstange] from comment #4)
> I've fixed my patch to actually send the hittest event with correct
> coordinates (the last patch was missing a conversion from screen coordinates
> to window coordinates), and the try results are somewhere between the two
> pushes in comment 3:
> https://tbpl.mozilla.org/?tree=Try&rev=084e069997f8

So, AIUI, the patch I pushed avoids this (optimized) code being run at all on XP, which is likely why it does still a bit better, and we are OK with this because there's no need to have native window move handling on XP, as there's nothing special like Aero Snap that's going on. However, if my understanding is correct and my patch makes sense, it's probably still worth having this because it'll improve the Win7/8 numbers a bit more, too? Is that right, Markus?

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Markus Stange [:mstange] from comment #4)
> > I've fixed my patch to actually send the hittest event with correct
> > coordinates (the last patch was missing a conversion from screen coordinates
> > to window coordinates), and the try results are somewhere between the two
> > pushes in comment 3:
> > https://tbpl.mozilla.org/?tree=Try&rev=084e069997f8
> 
> So, AIUI, the patch I pushed avoids this (optimized) code being run at all
> on XP, which is likely why it does still a bit better, and we are OK with
> this because there's no need to have native window move handling on XP, as
> there's nothing special like Aero Snap that's going on. However, if my
> understanding is correct and my patch makes sense, it's probably still worth
> having this because it'll improve the Win7/8 numbers a bit more, too? Is
> that right, Markus?

Also worth noting, perhaps, that while this improves ts_dirtypaint, it seems that tpaint isn't affected, whereas it is with the patch I pushed earlier.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 779691 [details] [diff] [review]
894099-optimize-client-hittest

Review of attachment 779691 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +5501,5 @@
> +    allowContentOverride = (mx >= winRect.left + nonClientSize.left &&
> +                            mx <= winRect.right - nonClientSize.right &&
> +                            my >= winRect.top + nonClientSize.top &&
> +                            my <= winRect.bottom - nonClientSize.bottom);
> +  }

I think this condition change is not exactly what we want because it's gonna return true for maximized mode, and even in maximized mode we don't need to care about the content override on WinXP.

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 779691 [details] [diff] [review]
894099-optimize-client-hittest

(with comment 7)

Comment 9

[Markus Stange [:mstange]]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Markus Stange [:mstange] from comment #4)
> > I've fixed my patch to actually send the hittest event with correct
> > coordinates (the last patch was missing a conversion from screen coordinates
> > to window coordinates), and the try results are somewhere between the two
> > pushes in comment 3:
> > https://tbpl.mozilla.org/?tree=Try&rev=084e069997f8
> 
> So, AIUI, the patch I pushed avoids this (optimized) code being run at all
> on XP, which is likely why it does still a bit better

Oh, right, I was a little confused by the reverse order of the embedded tbpls in your comment (inserted by bugzilla-js)...

> However, if my
> understanding is correct and my patch makes sense, it's probably still worth
> having this because it'll improve the Win7/8 numbers a bit more, too? Is
> that right, Markus?

Exactly. Looks like it's only about 1 or 2%, but probably still worth taking.

Comment 10

[:Gijs (he/him)]

Attachment: Optimize client hittest

OK, so this looked really good on try, so I'd like to get this on UX. Felipe, can you review the toolkit part, and Jim, can you review the native part? Thanks!

Comment 11

[:Gijs (he/him)]

Attachment: Optimize client hittest

Correct patch this time... Sorry for the spam.

Comment 12

[:Gijs (he/him)]

Note to self: if/when this lands, take the blame away from MattN. :-)

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 779989 [details] [diff] [review]
Optimize client hittest

Review of attachment 779989 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/nsWindow.cpp
@@ +5495,5 @@
>                              std::max(mHorResizeMargin - mNonClientOffset.left,
>                                       kResizableBorderMinSize));
>  
> +  bool allowContentOverride = false;
> +  // Only allow content overrides on pre-Vista Windows.

s/pre/post/

But it'd better to add the explanation of why. Something like "we can skip content hit testing on pre-vista Windows because they don't have Aero Snap so we're not interested in OS integration"

Comment 14

[Jim Mathies [:jimm]]

Does this break titlebar drag on XP? Setting allowContentOverride to false on Vista and up does.

Comment 15

[:Gijs (he/him)]

(In reply to Jim Mathies [:jimm] from comment #14)
> Does this break titlebar drag on XP? Setting allowContentOverride to false
> on Vista and up does.

Not as far as I can tell, neither for drawintitlebar (the default) nor for popup windows that don't have that (toolbars=no). Tested with the build from the try push in comment 3. Probably because I've also changed the dragging JSM to not return early for non-panels when we're not hittesting.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

(In reply to Jim Mathies [:jimm] from comment #14)
> Does this break titlebar drag on XP? Setting allowContentOverride to false
> on Vista and up does.

No, the idea is that XP will get titlebar drag done by WindowDraggingUtils.jsm through the mousedown/move/up handlers. I hope that works well enough so we can skip this hit testing code on XP

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 779989 [details] [diff] [review]
Optimize client hittest

Ok, functionally this is fine. If anything breaks we're sure to hear about it.

Comment 18

[:Gijs (he/him)]

Comment on attachment 779989 [details] [diff] [review]
Optimize client hittest

Pushed: https://hg.mozilla.org/projects/ux/rev/d6623f06fe55

Comment 19

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Felipe Gomes from comment #13)
> Comment on attachment 779989 [details] [diff] [review]
> Optimize client hittest
> 
> Review of attachment 779989 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/nsWindow.cpp
> @@ +5495,5 @@
> >                              std::max(mHorResizeMargin - mNonClientOffset.left,
> >                                       kResizableBorderMinSize));
> >  
> > +  bool allowContentOverride = false;
> > +  // Only allow content overrides on pre-Vista Windows.
> 
> s/pre/post/
> 
> But it'd better to add the explanation of why. Something like "we can skip
> content hit testing on pre-vista Windows because they don't have Aero Snap
> so we're not interested in OS integration"

This comment wasn't addressed in the patch that landed. Gijs, can you please push a follow-up with a better comment?

Comment 20

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #19)
> (In reply to :Felipe Gomes from comment #13)
> > Comment on attachment 779989 [details] [diff] [review]
> > Optimize client hittest
> > 
> > Review of attachment 779989 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/windows/nsWindow.cpp
> > @@ +5495,5 @@
> > >                              std::max(mHorResizeMargin - mNonClientOffset.left,
> > >                                       kResizableBorderMinSize));
> > >  
> > > +  bool allowContentOverride = false;
> > > +  // Only allow content overrides on pre-Vista Windows.
> > 
> > s/pre/post/
> > 
> > But it'd better to add the explanation of why. Something like "we can skip
> > content hit testing on pre-vista Windows because they don't have Aero Snap
> > so we're not interested in OS integration"
> 
> This comment wasn't addressed in the patch that landed. Gijs, can you please
> push a follow-up with a better comment?

I suck and should know not to push patches when I'm this tired and temperatures are this high. Apologies. Comment fixed in https://hg.mozilla.org/projects/ux/rev/0a565c255a8b

Comment 21

[Dão Gottwald [:dao]]

Does this patch not apply on mozilla-central? It's not clear to me what's UX-branch specific about it.

Comment 22

[Matthew N. [:MattN]]

It could probably have landed on m-c but I believe it would have no visible effect on Talos because I believe this code is only run when @chromemargin is set. tpaint and ts_paint run in restored Windows and chromemargin is only set for restored Windows on XP since bug 813802. We could land on m-c as well to help find regressions sooner but I don't feel strongly about this.

Comment 23

[Dão Gottwald [:dao]]

(In reply to Matthew N. [:MattN] from comment #22)
> It could probably have landed on m-c but I believe it would have no visible
> effect on Talos because I believe this code is only run when @chromemargin
> is set. tpaint and ts_paint run in restored Windows and chromemargin is only
> set for restored Windows on XP since bug 813802. We could land on m-c as
> well to help find regressions sooner but I don't feel strongly about this.

If there's a real performance win, we should probably ship it even if talos won't see it.

Comment 24

[Dão Gottwald [:dao]]

Comment on attachment 779989 [details] [diff] [review]
Optimize client hittest

>+#ifdef MOZ_WIDGET_COCOA
>+let useHitTest = true;
>+#else
>+let useHitTest = false;
>+#endif
>+
> #ifdef XP_WIN
>-#define USE_HITTEST
>-#elifdef MOZ_WIDGET_COCOA
>-#define USE_HITTEST
>+let hitTestUsageUpdated = false;
>+function updateHitTestUsage() {
>+  if (!hitTestUsageUpdated) {
>+    let sysInfo = Components.classes["@mozilla.org/system-info;1"]
>+                  .getService(Components.interfaces.nsIPropertyBag2);
>+    useHitTest = parseFloat(sysInfo.getProperty("version")) >= 6;
>+    hitTestUsageUpdated = true;
>+  }
>+}
> #endif

> this.WindowDraggingElement = function WindowDraggingElement(elem) {
>+#ifdef XP_WIN
>+  updateHitTestUsage();
>+#endif

This seems unnecessarily complicated. useHitTest could just be a smart getter.

Comment 25

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #24)
> Comment on attachment 779989 [details] [diff] [review]
> Optimize client hittest
> 
> This seems unnecessarily complicated. useHitTest could just be a smart
> getter.

I don't fully grok *exactly* what you mean by 'smart getter', and/or how that would be significantly less complicated. Sorry for being dense. :-(

I avoided a getter (even with caching) because that'd make event handlers slower by adding function calls (which were previously just #ifdefs). If you think that doesn't warrant the (in my view, limited - but I don't know how simple you can make your getter!) extra complexity, please file a followup.

(In reply to Dão Gottwald [:dao] from comment #23)
> (In reply to Matthew N. [:MattN] from comment #22)
> > It could probably have landed on m-c but I believe it would have no visible
> > effect on Talos because I believe this code is only run when @chromemargin
> > is set. tpaint and ts_paint run in restored Windows and chromemargin is only
> > set for restored Windows on XP since bug 813802. We could land on m-c as
> > well to help find regressions sooner but I don't feel strongly about this.
> 
> If there's a real performance win, we should probably ship it even if talos
> won't see it.

I sort of see your point, although I suspect the number of XP users who've voluntarily enabled drawintitlebar on our released builds isn't really worth it. If you still want to ship this on m-c, I think we should do it in one go with your suggested refactoring, should you still want to go ahead with that, to minimize churn. If after reading the above you don't think using a getter is worth it, please let me know and I can also land this on fx-team as-is.

Comment 26

[Dão Gottwald [:dao]]

A smart getter deletes and overwrites itself with a static value when it's accessed for the first time. There's nothing to cache, nothing to keep track of and nothing preventing you from utilizing #ifdef in the getter. You can do this with plain Javascript (e.g. __defineGetter__ and 'delete') or the XPCOMUtils.defineLazyGetter helper.

Comment 27

[Dão Gottwald [:dao]]

Or even better yet, since the getter would be accessed shortly after the jsm was loaded anyway: Just check the Windows version right where you declare useHitTest in the global scope.

Comment 28

[:Gijs (he/him)]

Attachment: simplify useHitTest setting

OK, that makes sense. Let's just do this, then. Caveat emptor: I haven't been able to test this properly because I can't build on Windows anymore on latest tip. However, I think the patch is straightforward enough, and it does compile on my mac...

Comment 29

[:Gijs (he/him)]

Comment on attachment 780427 [details] [diff] [review]
simplify useHitTest setting

https://hg.mozilla.org/projects/ux/rev/cbf5e9ebd287

Comment 30

[:Gijs (he/him)]

Attachment: Use WindowDraggingUtils.jsm for dragging the window on XP because the hit test is slow,

Patch for checkin on fx-team

Comment 31

[:Gijs (he/him)]

Comment on attachment 780829 [details] [diff] [review]
Use WindowDraggingUtils.jsm for dragging the window on XP because the hit test is slow,

Landed with patch user fixed. https://hg.mozilla.org/integration/fx-team/rev/0593679c47a9

Comment 32

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #31)
> Comment on attachment 780829 [details] [diff] [review]
> Use WindowDraggingUtils.jsm for dragging the window on XP because the hit
> test is slow,
> 
> Landed with patch user fixed.
> https://hg.mozilla.org/integration/fx-team/rev/0593679c47a9

Backed out of fx-team for bug 897901.

https://hg.mozilla.org/integration/fx-team/rev/c99eac4a75c6

Comment 33

[Dão Gottwald [:dao]]

Comment on attachment 780829 [details] [diff] [review]
Use WindowDraggingUtils.jsm for dragging the window on XP because the hit test is slow,

This patch started with the assumption that MozMouseHittest is only good for Aero Snap and otherwise redundant on Windows. This was just wrong. I should have known better, sorry. Please back this out from the UX branch as well.

For the purpose of this bug, we should instead make it so that hit testing doesn't affect window opening performance at all, because the fact that it does is really stupid. I suppose we can do this in the back-end or in the front-end by adding the event listener later. Hopefully this will be a more general perf win and not just for XP.

Comment 34

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #33)
> Comment on attachment 780829 [details] [diff] [review]
> Use WindowDraggingUtils.jsm for dragging the window on XP because the hit
> test is slow,
> 
> This patch started with the assumption that MozMouseHittest is only good for
> Aero Snap and otherwise redundant on Windows. This was just wrong. I should
> have known better, sorry. Please back this out from the UX branch as well.
> 
> For the purpose of this bug, we should instead make it so that hit testing
> doesn't affect window opening performance at all, because the fact that it
> does is really stupid. I suppose we can do this in the back-end or in the
> front-end by adding the event listener later. Hopefully this will be a more
> general perf win and not just for XP.

Backed out: https://hg.mozilla.org/projects/ux/rev/eb94d597ec47

Comment 35

[:Felipe Gomes (needinfo for replies!)]

MozMouseHittest is for OS integration, which Aero Snap is the most important thing that we can't implement by ourselves. But the dblclick to maximize and the system menu can be implemented in the front-end (see bug 897901), so if we ultimately need this patch, it's not impossible to take.

Granted, if there are simpler solutions that mitigate the perf impact of MozMouseHittest during window opening, we should look at those first. (I think the caching of the last result for the last position for 1sec will be enough to do it)

Comment 36

[Dão Gottwald [:dao]]

Given bug 898126, there's probably no point in keeping this open any longer. If that bug still left room for improvement, that could be addressed by initially caching ClientHitTest values more aggressively to not interfere with the window opening process.