The default bug view has changed. See this FAQ.

Prevent firing timers from lock contention with the main thread

RESOLVED FIXED in mozilla13

Status

()

Core
XPCOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla13
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Snappy:P1])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
Attachment #603148 - Flags: review?(benjamin)
(Assignee)

Updated

5 years ago
Assignee: nobody → ehsan
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.
Attachment #603148 - Flags: review?(benjamin) → review-
(Assignee)

Comment 2

5 years ago
(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?
(Assignee)

Comment 3

5 years ago
Created attachment 603760 [details] [diff] [review]
Patch (v2)
Attachment #603148 - Attachment is obsolete: true
Attachment #603760 - Flags: review?(benjamin)

Updated

5 years ago
Whiteboard: [Snappy] → [Snappy:P1]
Attachment #603760 - Flags: review?(benjamin) → review+
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3365dff8c5a
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/e3365dff8c5a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 799142
You need to log in before you can comment on or make changes to this bug.