Closed Bug 894099 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Widget: Win32, defect)

All
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: MattN, Assigned: Gijs)

References

Details

(Keywords: perf, Whiteboard: [Australis:M8])

Attachments

(3 files, 3 obsolete files)

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
Updated try push after the Try server reset: https://tbpl.mozilla.org/?tree=Try&rev=d341ee4efa6d
Attached patch Patch MattN ran against try (obsolete) — Splinter Review
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.
Attached patch 894099-optimize-client-hittest (obsolete) — Splinter Review
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 )
Attachment #779691 - Flags: feedback?(felipc)
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
(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?
Flags: needinfo?(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, 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 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 on attachment 779691 [details] [diff] [review]
894099-optimize-client-hittest

(with comment 7)
Attachment #779691 - Flags: feedback?(felipc) → feedback+
(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.
Flags: needinfo?(mstange)
Attached patch Optimize client hittest (obsolete) — Splinter Review
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!
Attachment #779988 - Flags: review?(jmathies)
Attachment #779988 - Flags: review?(felipc)
Attachment #779691 - Attachment is obsolete: true
Assignee: mstange → gijskruitbosch+bugs
Correct patch this time... Sorry for the spam.
Attachment #779989 - Flags: review?(jmathies)
Attachment #779989 - Flags: review?(felipc)
Attachment #779988 - Attachment is obsolete: true
Attachment #779988 - Flags: review?(jmathies)
Attachment #779988 - Flags: review?(felipc)
Note to self: if/when this lands, take the blame away from MattN. :-)
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"
Attachment #779989 - Flags: review?(felipc) → review+
Does this break titlebar drag on XP? Setting allowContentOverride to false on Vista and up does.
(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.
(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 on attachment 779989 [details] [diff] [review]
Optimize client hittest

Ok, functionally this is fine. If anything breaks we're sure to hear about it.
Attachment #779989 - Flags: review?(jmathies) → review+
Comment on attachment 779989 [details] [diff] [review]
Optimize client hittest

Pushed: https://hg.mozilla.org/projects/ux/rev/d6623f06fe55
Attachment #779989 - Flags: checkin+
(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?
Flags: needinfo?(gijskruitbosch+bugs)
(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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #779659 - Attachment is obsolete: true
Summary: Optimize nsWindow::ClientMarginHitTestPoint → Use WindowDraggingUtils.jsm for dragging the window on XP because the hit test is slow
Whiteboard: [fixed-in-ux]
Whiteboard: [fixed-in-ux] → [Australis:M8][fixed-in-ux]
Does this patch not apply on mozilla-central? It's not clear to me what's UX-branch specific about it.
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.
(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 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.
(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.
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.
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.
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...
Attachment #780427 - Flags: review?(dao)
Attachment #780427 - Flags: review?(dao) → review+
Attachment #779989 - Attachment is obsolete: true
Attachment #779989 - Attachment is obsolete: false
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
Attachment #780829 - Flags: checkin+
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8][fixed-in-ux][fixed-in-fxteam]
Depends on: 897901
(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
Whiteboard: [Australis:M8][fixed-in-ux][fixed-in-fxteam] → [Australis:M8][fixed-in-ux]
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.
Attachment #780829 - Flags: review-
(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
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)
Depends on: 898126
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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Whiteboard: [Australis:M8][fixed-in-ux] → [Australis:M8]
Attachment #779989 - Flags: checkin+
Attachment #780829 - Flags: checkin+
Attachment #780427 - Flags: checkin+
You need to log in before you can comment on or make changes to this bug.