Last Comment Bug 733277 - Prevent firing timers from lock contention with the main thread
: Prevent firing timers from lock contention with the main thread
Status: RESOLVED FIXED
[Snappy:P1]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla13
Assigned To: :Ehsan Akhgari
:
Mentors:
Depends on: 799142
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-05 20:18 PST by :Ehsan Akhgari
Modified: 2013-03-27 00:01 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (5.37 KB, patch)
2012-03-05 20:18 PST, :Ehsan Akhgari
benjamin: review-
Details | Diff | Splinter Review
Patch (v2) (5.37 KB, patch)
2012-03-07 09:42 PST, :Ehsan Akhgari
benjamin: review+
Details | Diff | Splinter Review

Description :Ehsan Akhgari 2012-03-05 20:18:25 PST
Created attachment 603148 [details] [diff] [review]
Patch (v1)

I've seen this problem over and over with my local profiles of Firefox.  What happens is that the timer thread allocates an object using operator new every time that it wants to send a timer event to the main thread.  The jemalloc allocation function tries to grab the lock which is also used for main thread allocations/deallocations.  This specially hurts during events which tend to allocate and deallocate a lot of objects, such as the CC.

My patch avoids this problem by using a separate fixed size allocator for these allocations, and only locking for those object types.
Comment 1 Benjamin Smedberg [:bsmedberg] 2012-03-06 13:18:14 PST
Comment on attachment 603148 [details] [diff] [review]
Patch (v1)

I'm surprised that there is noticeable contention here... you measured it using vtune or something that actually measures lock contention specifically?

I think that at least nsTimerEvent::Shutdown needs to happen *after* gThread->Shutdown, because otherwise can't you race?

I think that you also need an in-code comment explaining why you're using a special allocator.
Comment 2 :Ehsan Akhgari 2012-03-07 09:00:37 PST
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> Comment on attachment 603148 [details] [diff] [review]
> Patch (v1)
> 
> I'm surprised that there is noticeable contention here... you measured it
> using vtune or something that actually measures lock contention specifically?

I've used Instruments to profile this.  The amount of contention really varies depending on the number of tabs you have loaded, the content in those tabs, etc, but in the worst cases I've seen us spending up to 70% of our time fighting over this lock when the main thread is doing something allocator intensive.  :(

> I think that at least nsTimerEvent::Shutdown needs to happen *after*
> gThread->Shutdown, because otherwise can't you race?

Hmm, yeah I guess that's possible.  I'll submit a new patch which fixes that issue.

> I think that you also need an in-code comment explaining why you're using a
> special allocator.

Do you mean something more than the comments I have before the declaration of TimerEventAllocator?
Comment 3 :Ehsan Akhgari 2012-03-07 09:42:05 PST
Created attachment 603760 [details] [diff] [review]
Patch (v2)
Comment 5 Marco Bonardo [::mak] 2012-03-13 04:52:44 PDT
https://hg.mozilla.org/mozilla-central/rev/e3365dff8c5a

Note You need to log in before you can comment on or make changes to this bug.