733277 - Prevent firing timers from lock contention with the main thread

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

Attachment: 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]

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

[(no longer active)]

(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

[(no longer active)]

Attachment: Patch (v2)

Comment 4

[(no longer active)]

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

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

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