Closed Bug 786789 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: General, defect)

18 Branch
x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: joseph.k.olivas, Assigned: joseph.k.olivas)

Details

Attachments

(2 files, 2 obsolete files)

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
Product: Firefox → Core
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.
I added an attachment, but it looks like I didn't mark it as a patch.
Attachment #656557 - Attachment is obsolete: true
Attachment #656569 - Flags: feedback?(ehsan)
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 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.
Attachment #656569 - Flags: feedback?(ehsan) → review+
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 joe@drew.ca review it?

Thanks!
Done with Bug 786883 

Thanks!
Status: UNCONFIRMED → NEW
Ever confirmed: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/5502417dfd4e
Assignee: nobody → joseph.k.olivas
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/5502417dfd4e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #658239 - Flags: review?(surkov.alexander)
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.
Attachment #658239 - Attachment is obsolete: true
Attachment #658239 - Flags: review?(surkov.alexander)
(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
You need to log in before you can comment on or make changes to this bug.