786789 - Blocked Store Forward in TimeDuration::ToSeconds in TimeStamp_windows.cpp

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Joe Olivas]

Attachment: Simplifying codegen for TimeDuration::ToSeconds

Running a multi-tab workload (8 tabs opened after each has loaded) shows that the hottest function is mozilla::TimeDuration::ToSeconds (6.92% of time in xul.dll). This can be fixed with a simple patch to convert the type before the arithmetic, and reduce the time to ~4.5% in xul.dll. Small gains do add up :)

The function is a one-liner:

return double(mValue) / (sFrequencyPerSec * 1000ULL);

and generates the following code flow with MSVC2010 (cleaned up for readability and following taken branches):

sub 	esp,8
fild 	st0,qword ptr [ecx]
mov 	eax,dword ptr [10f61704]
mov 	ecx,dword ptr [10f61700]
push 	10001000
push 	100013e8
push 	eax
push 	ecx
call 	_allmul
mov 	eax,dword ptr [esp+8]
mov 	ecx,dword ptr [esp+10]
or 	ecx,eax
mov 	ecx,dword ptr [esp+c]
jnz 	10c239d9
mov 	eax,dword ptr [esp+4]
mul 	ecx
ret 	10001010
mov 	dword ptr [esp],eax <- 32 bit store
mov 	eax,edx
and 	edx,7fffffff
mov 	dword ptr [esp+4],edx <- 32-bit store
fild 	st0,qword ptr [esp] <- 64-bit load. Store forwarding blocked
and 	eax,80000000
mov 	dword ptr [esp+4],eax <- 32 bit store
mov 	dword ptr [esp],0 <- 32 bit store
fild 	st0,qword ptr [esp] <- 64-bit load. Store forwarding blocked
fchs 	st0
faddp 	st1,st0
fdivp 	st1,st0
add 	esp,8
ret

Meaning not only is there a lot of arithmetic overhead, but the stores marked above need to complete before they can be loaded. In most modern architectures, stores that are immediately loaded are forwarded in the pipeline, which eliminates the dependency on the store needing to complete. Changing the function to convert the types before the arithmetic like so:

return double(mValue) / (double(sFrequencyPerSec) * 1000.0);

generates a much simpler and faster function that can be inlined to reduce call/ret overhead:

fild 	st0,qword ptr [ecx]
fild 	st0,qword ptr [10f61700]
fmul 	st0,qword ptr [10c58568]
fdivp 	st1,st0
ret

Comment 1

[Josh Matthews [:jdm]]

Thank you for this investigation, Joe! If you have a patch prepared, it would be great to attach it to this bug for a proper review.

Comment 2

[Joe Olivas]

I added an attachment, but it looks like I didn't mark it as a patch.

Comment 3

[Joe Olivas]

Attachment: Simplifying codegen for TimeDuration::ToSeconds

Comment 4

[Joe Olivas]

Attachment: Reduce the calls into TimeDuration::ToSeconds

It appears a better (through complementary) fix is just not to make the call into this function as often.

Patch to use a TimeStamp object with a deadline to check against Timestamp::Now rather than calling into TimeDuration::ToSeconds so often.

Comment 5

[(no longer active)]

Comment on attachment 656569 [details] [diff] [review]
Simplifying codegen for TimeDuration::ToSeconds

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

This looks great, Joe!  Thanks a lot for the investigation and for your patch!  :-)  I will commit this patch right now.

Comment 6

[(no longer active)]

Comment on attachment 656655 [details] [diff] [review]
Reduce the calls into TimeDuration::ToSeconds

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

This patch looks fine to me, but I prefer someone who's more familiar with that code to review it.  Can you please file a new bug, attach this patch there and ask [e-mail] review it?

Thanks!

Comment 7

[Joe Olivas]

Done with Bug 786883 

Thanks!

Comment 8

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5502417dfd4e

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5502417dfd4e

Comment 10

[Trevor Saunders (:tbsaunde)]

Attachment: patch

This is just moving stuff where there is a method already on DocAccessible that isn't just a wrapper so I think we can agree on these.

Comment 11

[alexander :surkov (:asurkov)]

Comment on attachment 658239 [details] [diff] [review]
patch

It's a mix of patch of bug 777603 and some another patch, it doesn't seem right.

Comment 12

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #11)
> Comment on attachment 658239 [details] [diff] [review]
> patch
> 
> It's a mix of patch of bug 777603 and some another patch, it doesn't seem
> right.

yeah, sorry all I'm a clown who should be more careful when using commit --amend