Closed Bug 1498651 Opened Last year Closed Last year

improve mutex handling in timer implementation

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file, 2 obsolete files)

jesup posted some mutex acquisition stats in bug 1498643.  We have ~7k nsTimerImpl::mMutex objects created, with nearly 500k acquisitions (!).  One mutex is responsible for about half of those, which seems pretty nonsensical.  But most of the mutexes have single-digit acquisitions over the life of the mutex.

But some of these acquisitions are wholly unnecessary.  Take this creation function, for example:

nsresult
NS_NewTimerWithObserver(nsITimer** aTimer,
                        nsIObserver* aObserver,
                        uint32_t aDelay,
                        uint32_t aType,
                        nsIEventTarget* aTarget)
{
  auto timer = MakeRefPtr<nsTimer>();
  if (aTarget) {
    MOZ_ALWAYS_SUCCEEDS(timer->SetTarget(aTarget));
  }

  MOZ_TRY(timer->Init(aObserver, aDelay, aType));
  timer.forget(aTimer);
  return NS_OK;
}

timer->SetTarget() will acquire the mutex, which is necessary in general, but not in this specific case, because the timer has not been shared to the outside world yet.  It would be better to call an unlocked variant here, or even to pass in the event target to a constructor function.

It may be worth examining call sites that call SetTarget() on newly-created timers to instead pass in the event target at creation, so we can avoid unnecessary locking.

Similarly, timer->Init() will call into InitCommon(), which requires the mutex to be held.  Again, we don't need the mutex in this particular case, because the timer hasn't been shared to the outside world yet.
Oh, ugh, actually InitCommon may require locking just because it interacts with the timer thread.  If we are careful, though, I think the locking might be unnecessary.
(In reply to Nathan Froyd [:froydnj] from comment #1)
> Oh, ugh, actually InitCommon may require locking just because it interacts
> with the timer thread.

That makes no sense though. If each nsTimerImpl has its mutex, all that's preventing is a single nsTimerImpl' Init* doing things at the same time as another thread using the *same nsTimerImpl*. Different nsTimerImpl instances can happily use gThread concurrently!
(In reply to Mike Hommey [:glandium] from comment #2)
> (In reply to Nathan Froyd [:froydnj] from comment #1)
> > Oh, ugh, actually InitCommon may require locking just because it interacts
> > with the timer thread.
> 
> That makes no sense though. If each nsTimerImpl has its mutex, all that's
> preventing is a single nsTimerImpl' Init* doing things at the same time as
> another thread using the *same nsTimerImpl*. Different nsTimerImpl instances
> can happily use gThread concurrently!

Other functions that twiddle state Init* uses (e.g. Cancel()) should be prevented from racing with Init*.  The process of firing the timer on the timer thread or its target thread should be prevented from racing with Init*.  You might say that this really shouldn't happen, and I would tend to agree with you, but we had a spate of security bugs with races in the timer implementation, and I am loath to relax things *too* much.

I think the *first* call to Init* in NS_NewTimer* ought to be lock-free, because nothing else has a handle on the timer.  We might have to have a memory barrier before passing the timer off to the timer thread to ensure that the timer thread can correctly see all of the timer's initialized fields, but that should be all the synchronization we need.  And I think that only works because Init* would be the last thing called before returning out of NS_NewTimer*.  (You could have the timer fire before Init() returns...I think?)

But I don't think I'd want to completely remove the lock on Init* without a lot of fuzz testing.
The NS_NewTimer* family of functions, when using a custom event target,
currently go through a path that looks something like:

  auto timer = createTimer()
  timer->SetTarget(target);
  // call the requisite Init* function
  return timer;

This setup is inefficient, because SetTarget requires the timer mutex to
be acquired.  The mutex acquisition here is completely unnecessary,
because the timer hasn't yet been shared out to the wider world; we can
set the timer target without acquiring the mutex at all because we know
that no sharing is possible at this point.

This patch reworks things somewhat to make that possible.  Other mutex bits can
be deferred to other bugs.
Attachment #9017859 - Flags: review?(erahm)
Better patch that doesn't fall all over itself on try.
Attachment #9017914 - Flags: review?(erahm)
Attachment #9017859 - Attachment is obsolete: true
Attachment #9017859 - Flags: review?(erahm)
Comment on attachment 9017859 [details] [diff] [review]
make initial timer target setting more efficient

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

Looks good, one minor yak shave.

::: xpcom/threads/nsTimerImpl.h
@@ +226,5 @@
>    virtual ~nsTimer();
>  public:
> +  // This constructor exists for the benefit of XPConnect integration; you
> +  // should be using NS_NewTimer* from C++.
> +  nsTimer() : nsTimer(nullptr) {};

It's unfortunate we can't make this private; it would be nice to prevent incorrect usage.

@@ +238,5 @@
>    virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const override;
>  
> +  // Create a timer targeting the given target.  nullptr indicates that the
> +  // current thread should be used as the timer's target.
> +  static RefPtr<nsTimer> WithEventTarget(nsIEventTarget* aTarget);

nit: maybe call this 'MakeWithEventTarget'. I don't feel too strongly about this.
Attachment #9017859 - Attachment is obsolete: false
Comment on attachment 9017914 [details] [diff] [review]
make initial timer target setting more efficient

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

Looks like we mid-aired, same comment as before.
Attachment #9017914 - Flags: review?(erahm) → review+
Attachment #9017859 - Attachment is obsolete: true
Made the xpcom-y constructor sufficiently ugly and obvious that nobody will
want to touch it.  Carrying over r+.
Attachment #9017958 - Flags: review+
Attachment #9017914 - Attachment is obsolete: true
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3714d8976238
make initial timer target setting more efficient; r=erahm
https://hg.mozilla.org/mozilla-central/rev/3714d8976238
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Assignee: nobody → nfroyd
You need to log in before you can comment on or make changes to this bug.