898126 - Cache/ignore ClientHitTest requests to improve window open times

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[:Gijs (out for now; he/him)]

As per discussion on IRC today, we can probably cache some of the OS's requests for NCHITTEST messages, because it spams us a lot. Markus has a patch for this which I'm about to push to try.

Separately, we can probably also safely do without really checking whether or not we want a native menu for anything further than about 200px down from the top of the window and short-circuit those requests. I'll try to do a patch for that and submit the combination to try to see how much that saves us.

Comment 1

[Dão Gottwald [:dao]]

We shouldn't be doing any work here before the first paint.

Comment 2

[:Gijs (out for now; he/him)]

Attachment: Cache client hit test values

https://tbpl.mozilla.org/?tree=Try&rev=353e84a5e820

Comment 3

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #1)
> We shouldn't be doing any work here before the first paint.

Logs from https://tbpl.mozilla.org/?tree=Try&rev=ae3873531f0f (eg. https://tbpl.mozilla.org/php/getParsedLog.php?id=25715072&tree=Try&full=1 ) show that we don't, sort of - that is, we get a request to paint, then do a whole bunch of stuff, then get another request to paint, then the tpaint test considers the window painted. It's the "whole bunch of stuff" inbetween the two paint (requests?) we're trying to reduce. I don't know if we actually paint but somehow don't fire moz_after_paint immediately after the first event. Markus or Jim might know more about that.

Comment 4

[:Gijs (out for now; he/him)]

(not getting to the px offset idea today anymore, will try again Europe time tomorrow, unless someone beats me to it)

Comment 5

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> Created attachment 781203 [details] [diff] [review]
> Cache client hit test values
> 
> https://tbpl.mozilla.org/?tree=Try&rev=353e84a5e820

Egh. Of course, if you're trying to get perf numbers, it helps if you take out the printfs. Will followup shortly. :-\

Comment 6

[:Gijs (out for now; he/him)]

Attachment: Cache client hit test values

Without printfs this time: https://tbpl.mozilla.org/?tree=Try&rev=b5bb83ac4873

Comment 7

[:Gijs (out for now; he/him)]

Attachment: Cache client hit test values

C++ is hard. Let's try that again: https://tbpl.mozilla.org/?tree=Try&rev=833411e6e74c

Comment 8

[:Gijs (out for now; he/him)]

So the initial patch shows about the same wins as bug 894099, without the functionality regressions. The 200px vertical maximum doesn't seem to reduce time spent there further, comparing:

https://tbpl.mozilla.org/?tree=Try&rev=833411e6e74c

and 

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

( http://perf.snarkfest.net/compare-talos/index.html?oldRevs=833411e6e74c&newRev=e9a522557760&submit=true )

So that's not worth it - but Markus, would you be willing to move the caching patch along?

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 781613 [details] [diff] [review]
Cache client hit test values

Quick background: It turns out that Windows XP sends about 200 WM_NCHITTEST events per second when we open a new window. All these events have the same position - possibly the current mouse position. And all the ClientMarginHitTestPoint optimizations we've been playing with only make a difference because that function is called so often during the test - one invocation is unnoticeably quick, but it starts to add up if we call it so many times.
This patch makes sure that we only send one hittest event per second if the position doesn't change, and returns a cached value otherwise. In theory, this makes the optimization from bug 897260 unnecessary, but I think we can keep it because caching a POINT looks nicer than caching an lParam. What do you think, Jim?

Comment 10

[Jim Mathies [:jimm]]

Do we need the time duration check? If the point handed to use hasn't change, would the result of ClientMarginHitTestPoint change over time?

Comment 11

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #10)
> Do we need the time duration check? If the point handed to use hasn't
> change, would the result of ClientMarginHitTestPoint change over time?

If anything in layout changed inbetween the two checks, the element underneath it could change, and the different element might / might not want to return the same value when we fire the hittest event.

Comment 12

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #11)
> (In reply to Jim Mathies [:jimm] from comment #10)
> > Do we need the time duration check? If the point handed to use hasn't
> > change, would the result of ClientMarginHitTestPoint change over time?
> 
> If anything in layout changed inbetween the two checks, the element
> underneath it could change, and the different element might / might not want
> to return the same value when we fire the hittest event.

Ok. So one second seems kind of arbitrary and long, could it be something like 100 msec instead which would give you 10 queries per second, assuming the coordinates don't change?

Comment 13

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #12)
> (In reply to :Gijs Kruitbosch from comment #11)
> > (In reply to Jim Mathies [:jimm] from comment #10)
> > > Do we need the time duration check? If the point handed to use hasn't
> > > change, would the result of ClientMarginHitTestPoint change over time?
> > 
> > If anything in layout changed inbetween the two checks, the element
> > underneath it could change, and the different element might / might not want
> > to return the same value when we fire the hittest event.
> 
> Ok. So one second seems kind of arbitrary and long, could it be something
> like 100 msec instead which would give you 10 queries per second, assuming
> the coordinates don't change?

Hrm. It'd probably diminish the performance win here.

The thing is, this won't really affect users, unless they were holding their mouse really still and clicked in the <1s timeperiod where something changed and we weren't up-to-date with it having changed. This is extremely unlikely if not impossible (in practice, not much changes in the titlebar, and you'd have a hard time hitting this if you weren't really trying). If the user is moving the mouse, we would still be re-sending-off the event. Does that make sense?

Comment 14

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Jim Mathies [:jimm] from comment #12)
> > (In reply to :Gijs Kruitbosch from comment #11)
> > > (In reply to Jim Mathies [:jimm] from comment #10)
> > > > Do we need the time duration check? If the point handed to use hasn't
> > > > change, would the result of ClientMarginHitTestPoint change over time?
> > > 
> > > If anything in layout changed inbetween the two checks, the element
> > > underneath it could change, and the different element might / might not want
> > > to return the same value when we fire the hittest event.
> > 
> > Ok. So one second seems kind of arbitrary and long, could it be something
> > like 100 msec instead which would give you 10 queries per second, assuming
> > the coordinates don't change?
> 
> Hrm. It'd probably diminish the performance win here.

Well, if we have performance problems with sending and processing 10 gecko events per second, we have far bigger problems in our code than in this little chunk. ;)

> The thing is, this won't really affect users, unless they were holding their
> mouse really still and clicked in the <1s timeperiod where something changed
> and we weren't up-to-date with it having changed. This is extremely unlikely
> if not impossible (in practice, not much changes in the titlebar, and you'd
> have a hard time hitting this if you weren't really trying). If the user is
> moving the mouse, we would still be re-sending-off the event. Does that make
> sense?

According to the WM_NCHITTEST docs, we may receive this in response to things other than mouse movement. So if our layout can change (and the result can change) in under one second, which it can, under what circumstances might we be returning invalid values? And what might be the side effects of that?

Comment 15

[Jim Mathies [:jimm]]

Also one other thought I had, if we receive 200 messages when creating a window, are we on the stack for those, or is that just Windows bad behavior?

Comment 16

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #14)
> (In reply to :Gijs Kruitbosch from comment #13)
> > (In reply to Jim Mathies [:jimm] from comment #12)
> > > (In reply to :Gijs Kruitbosch from comment #11)
> > > > (In reply to Jim Mathies [:jimm] from comment #10)
> > > > > Do we need the time duration check? If the point handed to use hasn't
> > > > > change, would the result of ClientMarginHitTestPoint change over time?
> > > > 
> > > > If anything in layout changed inbetween the two checks, the element
> > > > underneath it could change, and the different element might / might not want
> > > > to return the same value when we fire the hittest event.
> > > 
> > > Ok. So one second seems kind of arbitrary and long, could it be something
> > > like 100 msec instead which would give you 10 queries per second, assuming
> > > the coordinates don't change?
> > 
> > Hrm. It'd probably diminish the performance win here.
> 
> Well, if we have performance problems with sending and processing 10 gecko
> events per second, we have far bigger problems in our code than in this
> little chunk. ;)

Actually, we're sending events by coordinate. Then Gecko has to figure out what element lives at that coordinate, which triggers a sync reflow. Doing that very often when we're busy trying to open the browser is understandably not great for performance, and we should avoid it, IMO.

(In reply to Jim Mathies [:jimm] from comment #15)
> Also one other thought I had, if we receive 200 messages when creating a
> window, are we on the stack for those, or is that just Windows bad behavior?

I can try and look but I don't have an environment on XP to easily get stacks, unless there's some way I can programmatically printf them (I'm building on Win7, then testing on an XP VM).

Comment 17

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #15)
> Also one other thought I had, if we receive 200 messages when creating a
> window, are we on the stack for those, or is that just Windows bad behavior?

Bit of both, apparently. I checked on Win7 (don't have a development env on Windows and don't understand how I can get my symbols from a->b) and I see two different stacks:

xul.dll!nsWindow::ClientMarginHitTestPoint() 
	xul.dll!nsWindow::ProcessMessage() 
	xul.dll!nsWindow::WindowProcInternal() 
	xul.dll!CallWindowProcCrashProtected() 
	xul.dll!nsWindow::WindowProc() 
	user32.dll!758b62fa() 
	[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]
	user32.dll!758b6d3a() 
	user32.dll!758b6ce9() 
	user32.dll!758b6de8() 
	user32.dll!758b6e44() 
	ntdll.dll!77e0010a() 
	user32.dll!758c0735() 
	user32.dll!758c06eb() 
	user32.dll!758c0751() 
	xul.dll!mozilla::widget::WinUtils::PeekMessageW() 
	xul.dll!nsAppShell::ProcessNextNativeEvent() 
	xul.dll!nsBaseAppShell::DoProcessNextNativeEvent() 
	xul.dll!nsBaseAppShell::OnProcessNextEvent() 
	xul.dll!nsThread::ProcessNextEvent() 
	xul.dll!NS_ProcessNextEvent() 
	xul.dll!mozilla::ipc::MessagePump::Run() 
	xul.dll!MessageLoop::RunInternal() 
	xul.dll!MessageLoop::RunHandler() 
	xul.dll!MessageLoop::Run() 
	xul.dll!nsBaseAppShell::Run() 
	xul.dll!nsAppShell::Run() 
	xul.dll!nsAppStartup::Run() 
	xul.dll!XREMain::XRE_mainRun() 
	xul.dll!XREMain::XRE_main() 
	xul.dll!XRE_main() 
	firefox.exe!do_main() 
	firefox.exe!NS_internal_main() 
	firefox.exe!wmain() 
	firefox.exe!__tmainCRTStartup() 
	kernel32.dll!763d33aa() 
	ntdll.dll!77e29ef2() 
	ntdll.dll!77e29ec5() 
	; Thread: 0x1298 Main Thread


xul.dll!nsWindow::ClientMarginHitTestPoint() 
	xul.dll!nsWindow::ProcessMessage() 
	xul.dll!nsWindow::WindowProcInternal() 
	xul.dll!CallWindowProcCrashProtected() 
	xul.dll!nsWindow::WindowProc() 
	user32.dll!758b62fa() 
	[Frames below may be incorrect and/or missing, no symbols loaded for user32.dll]
	user32.dll!758b6d3a() 
	user32.dll!758b6ce9() 
	user32.dll!758b6de8() 
	user32.dll!758b6e44() 
	ntdll.dll!77e0010a() 
	user32.dll!758ded27() 
	xul.dll!MouseTrailer::TimerProc() 
	800d0008()
	; Thread: 0x1298 Main Thread

It's hard for me to understand which is shown in which case. However, digging into the last one, it looks like this:

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsToolkit.cpp#185

Doing ::WindowFromPoint presumably triggers this.

I don't know if/how we could just avoid doing this, because I don't know this code and it's 00:21am here, and my brain is fried. I also don't know if that call is the actual cause of our problems - AFAICT the top call happens more when the window is being opened, the latter when it is already opened. I also don't know if the pattern I'm seeing on Win7 is the same as the one on XP.

Comment 18

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Jim Mathies [:jimm] from comment #15)
> > Also one other thought I had, if we receive 200 messages when creating a
> > window, are we on the stack for those, or is that just Windows bad behavior?
> 
> Bit of both, apparently. I checked on Win7 (don't have a development env on
> Windows

Typing fail: don't have a dev env on Windows *XP*

Comment 19

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Jim Mathies [:jimm] from comment #12)
> > Ok. So one second seems kind of arbitrary and long, could it be something
> > like 100 msec instead which would give you 10 queries per second, assuming
> > the coordinates don't change?
> 
> Hrm. It'd probably diminish the performance win here.

Could we start with a longer interval and then reduce it? Or are you concerned about the general performance impact rather than only during window creation?

Comment 20

[:Gijs (out for now; he/him)]

(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #13)
> > (In reply to Jim Mathies [:jimm] from comment #12)
> > > Ok. So one second seems kind of arbitrary and long, could it be something
> > > like 100 msec instead which would give you 10 queries per second, assuming
> > > the coordinates don't change?
> > 
> > Hrm. It'd probably diminish the performance win here.
> 
> Could we start with a longer interval and then reduce it? Or are you
> concerned about the general performance impact rather than only during
> window creation?

I think we should take whatever we can, but the decreasing interval option sounds OK to me.

In principle, I see no reason the calls coming through the mouse handler (which AFAICT is just checking whether the mouse is still in the window? Or something...) need this info particularly, so we could fast-path that if we knew where it came from (might be as trivial as setting something in the mouse proc handler when we call the Windows API?).

All the other calls likely need more investigation. It's hard to be sure when you get to multiple calls per second, but my impression is that I see fewer of these on Win7 than I did on XP, too. I guess I could test that assumption (or check the tpaint logs from the earlier try push with logging).

Comment 21

[:Gijs (out for now; he/him)]

Attachment: when triggered by the MouseTrailer, don't bother checking client hit test values.

So this works, in my testing, and gets me only a couple of messages (strictly from Windows) when I open a new window, in some quick tests on Win7. Jim, does this look anywhere near-decent? Please be ruthless with my C++, it's not my strong suit.

Comment 22

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #20)
> All the other calls likely need more investigation. It's hard to be sure
> when you get to multiple calls per second, but my impression is that I see
> fewer of these on Win7 than I did on XP, too. I guess I could test that
> assumption (or check the tpaint logs from the earlier try push with logging).

I think this was just due to the debugger being attached and therefore there were fewer calls per second. I can see if I have time later this weekend to doublecheck the outcomes of this particular patch on XP (in any case, try is closed right now).

Comment 23

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #22)
> (In reply to :Gijs Kruitbosch from comment #20)
> > All the other calls likely need more investigation. It's hard to be sure
> > when you get to multiple calls per second, but my impression is that I see
> > fewer of these on Win7 than I did on XP, too. I guess I could test that
> > assumption (or check the tpaint logs from the earlier try push with logging).
> 
> I think this was just due to the debugger being attached and therefore there
> were fewer calls per second. I can see if I have time later this weekend to
> doublecheck the outcomes of this particular patch on XP (in any case, try is
> closed right now).

https://tbpl.mozilla.org/?tree=Try&rev=7ed49f2414b7

(can use https://tbpl.mozilla.org/?tree=Try&rev=5b5c93f0f31f as a baseline)

Comment 24

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #23)
> https://tbpl.mozilla.org/?tree=Try&rev=7ed49f2414b7
> 
> (can use https://tbpl.mozilla.org/?tree=Try&rev=5b5c93f0f31f as a baseline)

Annnnd this seems to be a wash on our tests, except perhaps for ts_dirtypaint on XP (win7/win8 are now permanently horked on talos, it seems - see bug 859571 / bug 886109 / bug 869285). Color me confused! I guess the windows-originating requests we get during the window's initial paint are still the thing what's causing the performance regression here? I can try to look into this further on Monday if nobody beats me to it.

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 782010 [details] [diff] [review]
when triggered by the MouseTrailer, don't bother checking client hit test values.

Interesting, so some of this is caused by us. This particular source though is triggered by a 200 msec timer which isn't a big part of the problem. Lets stick with mstange's original work here and file a follow up on toolkit's mouse trailer behaviorm, or better yet investigate fixing bug 506815. :)

If that WindowFromPoint call we make is our only offense in this, I'm ok with the work mstange posted. I don't see any issue with throttling the result calculation of the hit test message since our gecko event processing can take time, and hundreds of sent messages during a one second period is something we should avoid. I think though we should try to match or come close to our preferred render frame rate in the throttle.

Comment 26

[Jim Mathies [:jimm]]

Also, if this is only an issue on xp, lets special case it using the WinUtils version checker.

Comment 27

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #26)
> Also, if this is only an issue on xp, lets special case it using the
> WinUtils version checker.

As far as I've been able to tell, this is an issue everywhere, although it might be slightly worse on XP (not entirely sure why). It's also a more pressing issue for Australis on XP, because m-c doesn't draw in the titlebar there by default. The fact that we enabled that triggers this problem, which is the prime responsible for our talos results being significantly worse on XP. On Win7/Win8, we draw in the titlebar on m-c also, so there's no such regression. Fixing (some of) this bug will cause tpaint and ts_paint improvements on win7/win8 on mc, and winxp/win7/win8 on ux. Does that make sense?

Comment 28

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #27)
> (In reply to Jim Mathies [:jimm] from comment #26)
> > Also, if this is only an issue on xp, lets special case it using the
> > WinUtils version checker.
> 
> As far as I've been able to tell, this is an issue everywhere, although it
> might be slightly worse on XP (not entirely sure why). It's also a more
> pressing issue for Australis on XP, because m-c doesn't draw in the titlebar
> there by default. The fact that we enabled that triggers this problem, which
> is the prime responsible for our talos results being significantly worse on
> XP. On Win7/Win8, we draw in the titlebar on m-c also, so there's no such
> regression. Fixing (some of) this bug will cause tpaint and ts_paint
> improvements on win7/win8 on mc, and winxp/win7/win8 on ux. Does that make
> sense?

Sure, sounds like we want to get this fixed.

Comment 29

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #25)
> Comment on attachment 782010 [details] [diff] [review]
> when triggered by the MouseTrailer, don't bother checking client hit test
> values.
> 
> Interesting, so some of this is caused by us. This particular source though
> is triggered by a 200 msec timer which isn't a big part of the problem. Lets
> stick with mstange's original work here and file a follow up on toolkit's
> mouse trailer behaviorm, or better yet investigate fixing bug 506815. :)

After some testing with logging as well as my hodgepodge patch to disable the mousetrailer-sourced ones, it appears that Windows itself is indeed also spamming us regularly with these, on both XP and Win7. We get a whole slew as the window is being opened, after which it becomes less crazy.

> If that WindowFromPoint call we make is our only offense in this, I'm ok
> with the work mstange posted. I don't see any issue with throttling the
> result calculation of the hit test message since our gecko event processing
> can take time, and hundreds of sent messages during a one second period is
> something we should avoid. I think though we should try to match or come
> close to our preferred render frame rate in the throttle.

I agree with this, I just wonder how best to do this. Do all paints go through the WM_PAINT stuff? From my logs, a quick test with CSS transitions, and some casual code inspection, I suspect this is the case, but as said I am a stranger in these lands. I might even have the wrong mental model for how this works entirely... :-)

Anyway, supposing WM_PAINT is an accurate guide to 'we now paint', could we throttle requests for the same coordinate within a single WM_PAINT cycle? That is, if the last request we got was for this coordinate and came after the last WM_PAINT, we should return the cached value rather than doing the same work over again.

Of course we could also use a similar system as mstange used, but with a shorter duration - but on window open time, where the problem seems biggest, we don't seem to be painting as much (presumably because we're a bit more busy?), and throttling to only 1s/60 or so wouldn't be as big a win.

Comment 30

[Jim Mathies [:jimm]]

> I agree with this, I just wonder how best to do this. Do all paints go
> through the WM_PAINT stuff? From my logs, a quick test with CSS transitions,
> and some casual code inspection, I suspect this is the case, but as said I
> am a stranger in these lands. I might even have the wrong mental model for
> how this works entirely... :-)

We manage painting internally through a refresh driver.
 
> Anyway, supposing WM_PAINT is an accurate guide to 'we now paint', could we
> throttle requests for the same coordinate within a single WM_PAINT cycle?
> That is, if the last request we got was for this coordinate and came after
> the last WM_PAINT, we should return the cached value rather than doing the
> same work over again.

overkill..

> Of course we could also use a similar system as mstange used, but with a
> shorter duration - but on window open time, where the problem seems biggest,
> we don't seem to be painting as much (presumably because we're a bit more
> busy?), and throttling to only 1s/60 or so wouldn't be as big a win.

If we throttled at say, 50msec, how big of a win does it give us? Can we push a patch like that to try for some talos runs to see?

Comment 31

[:Gijs (out for now; he/him)]

(In reply to Jim Mathies [:jimm] from comment #30)
> > I agree with this, I just wonder how best to do this. Do all paints go
> > through the WM_PAINT stuff? From my logs, a quick test with CSS transitions,
> > and some casual code inspection, I suspect this is the case, but as said I
> > am a stranger in these lands. I might even have the wrong mental model for
> > how this works entirely... :-)
> 
> We manage painting internally through a refresh driver.
>  
> > Anyway, supposing WM_PAINT is an accurate guide to 'we now paint', could we
> > throttle requests for the same coordinate within a single WM_PAINT cycle?
> > That is, if the last request we got was for this coordinate and came after
> > the last WM_PAINT, we should return the cached value rather than doing the
> > same work over again.
> 
> overkill..

I don't quite understand why. This just means 'nulling out' (to -1,-1/0,0) the cached coordinate on WM_PAINT (and possibly WM_NCPAINT). If anything, it eliminates the need for the timekeeping. In fact, I've already got a patch ready that does this (just finished testing on XP).

> > Of course we could also use a similar system as mstange used, but with a
> > shorter duration - but on window open time, where the problem seems biggest,
> > we don't seem to be painting as much (presumably because we're a bit more
> > busy?), and throttling to only 1s/60 or so wouldn't be as big a win.
> 
> If we throttled at say, 50msec, how big of a win does it give us? Can we
> push a patch like that to try for some talos runs to see?

Sure, I can do that, too.

Comment 32

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch from comment #31)
> (In reply to Jim Mathies [:jimm] from comment #30)
> > If we throttled at say, 50msec, how big of a win does it give us? Can we
> > push a patch like that to try for some talos runs to see?
> 
> Sure, I can do that, too.

50ms throttling:

https://tbpl.mozilla.org/?tree=Try&rev=28d357e5d47f

Throttling until paint:

https://tbpl.mozilla.org/?tree=Try&rev=34d1d7f8adf9

Baseline:

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

Comment 33

[:Gijs (out for now; he/him)]

Talos results:

50ms caching:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=d97ecf07d39a&newRev=28d357e5d47f&submit=true

cache until next paint:
http://perf.snarkfest.net/compare-talos/index.html?oldRevs=d97ecf07d39a&newRev=34d1d7f8adf9&submit=true

(note that noise is pretty much the cause of any of the red you're seeing; oh, and not retriggering tresize)

The latter shows a /slightly/ better win, but as Markus pointed out over IRC, it has the downside that for OMTC, this might no longer be a viable option. Jim, are you OK with either of these two options and/or do you want us to try something else?

Comment 34

[Jim Mathies [:jimm]]

(In reply to :Gijs Kruitbosch from comment #33)
> Talos results:
> 
> 50ms caching:
> http://perf.snarkfest.net/compare-talos/index.
> html?oldRevs=d97ecf07d39a&newRev=28d357e5d47f&submit=true
> 
> cache until next paint:
> http://perf.snarkfest.net/compare-talos/index.
> html?oldRevs=d97ecf07d39a&newRev=34d1d7f8adf9&submit=true
> 
> (note that noise is pretty much the cause of any of the red you're seeing;
> oh, and not retriggering tresize)
> 
> The latter shows a /slightly/ better win, but as Markus pointed out over
> IRC, it has the downside that for OMTC, this might no longer be a viable
> option. Jim, are you OK with either of these two options and/or do you want
> us to try something else?

I like the simple, straight forward 50ms throttle personally.

Comment 35

[:Gijs (out for now; he/him)]

Attachment: Cache client hit test values

Alright. Note that these talos runs were really noisy, but it looks like we might not get quite as much benefit as we did with the 1 second timeout. :-(

Comment 36

[:Gijs (out for now; he/him)]

Markus, I've pushed this forward because we need this rather desperately to be able to get Australis' perf sorted and have it merged ASAP. I've kept your name on the cset, I hope that's OK? :-)

Comment 37

[Markus Stange [:mstange]]

Sure. Thanks for finishing it up!

Comment 38

[Jim Mathies [:jimm]]

Comment on attachment 782660 [details] [diff] [review]
Cache client hit test values

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

We can always tweak the timeout over time. Let's start with something safe and conservative. Thanks for working on this!

::: widget/windows/nsWindow.cpp
@@ +5566,4 @@
>      POINT pt = { mx, my };
>      ::ScreenToClient(mWnd, &pt);
> +    if (pt.x == mCachedHitTestPoint.x && pt.y == mCachedHitTestPoint.y &&
> +        TimeStamp::Now() - mCachedHitTestTime < TimeDuration::FromMilliseconds(50)) {

nit - please move this 50 msec value up to the top of the file with nice comment explaining what it controls and why.

Comment 39

[:Gijs (out for now; he/him)]

Attachment: Cache client hit test values

Just checking this is more or less what you meant, Jim?

Comment 40

[Jim Mathies [:jimm]]

Comment on attachment 782687 [details] [diff] [review]
Cache client hit test values

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

perfect!

Comment 41

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b84cfa24ba5f

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

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b84cfa24ba5f

Comment 43

[:Gijs (out for now; he/him)]

https://hg.mozilla.org/mozilla-central/rev/e2fa6332a907