Hmm, now that I look at nsBaseHashtable::GetOrInsert() it seems like it's actually doing two lookups anyway... I filed bug 1371061.

BTW, just to clarify the patch a bit: I'm intentionally using "nsCOMPtr<nsITimer>" without a & to hold a strong ref on the stack during the call to Cancel() since it's what the existing code does.

Comment on attachment 8875463 [details] [diff] [review] Use mUpdateTimers.LookupRemoveIf() to avoid a second hashtable lookup for Remove(). Use mUpdateTimers.GetOrInsert() to avoid a second hashtable lookup for Put() Hmm, since the new code here depends on Cancel() not touching the hashtable we might as well use nsCOMPtr<nsITimer>& ...

I guess we can avoid the Remove calls I introduce here if we add a EntryPtr::Remove or some such (and use LookupForAdd instead of GetOrInsert), but these are failure cases that should be very rare. So it doesn't seem worth it.

This actually introduces a false positive assertion in debug builds. I'll explain and fix that in the next part...

In RemoveAll() we iterate mRegistrationInfos - this opens a PLDHashTable ReadOp for the duration of the iterator. Inside that iteration we call ForceUnregister, which calls Unregister, which calls GetOrCreateJobQueue, which does a hashtable lookup on mRegistrationInfos. Before part 2, this was a Get() which always finds an existing entry, so the Put() was not executed. A Get() counts as a ReadOp, so all is well. However, LookupForAdd calls PutEntry internally, which counts as a WriteOp, because it *might* do a write. It never does that on this path, but PLDHashTable::Add does an *unconditional* AutoWriteOp whether or not an actual write occurs or not, so it asserts: http://searchfox.org/mozilla-central/rev/a798ee4fc323f9387b7576dbed177859d29d09b7/xpcom/ds/PLDHashTable.cpp#541 So, this patch changes ForceUnregister to do the essential part of GetOrCreateJobQueue upfront, without accessing mRegistrationInfos at all. This avoids the false positive assertion. It also saves a hashtable lookup and unnecessary string copying (all of which occurs inside the nested loops of RemoveAll), so this should also be an additional performance win.

Comment on attachment 8876259 [details] [diff] [review] part 3 - Refactor Unregister/ForceUnregister to avoid unnecessary hashtable lookups and string copying. Review of attachment 8876259 [details] [diff] [review]: ----------------------------------------------------------------- This patch is a little more complex than mere hashtable operation adjustment, and I don't think I'm the right reviewer for it.

Comment on attachment 8876259 [details] [diff] [review] part 3 - Refactor Unregister/ForceUnregister to avoid unnecessary hashtable lookups and string copying. Review of attachment 8876259 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ServiceWorkerManager.cpp @@ +3684,5 @@ > + // Ensure we pass a non-null queue to avoid calling GetOrCreateJobQueue > + // in UnregisterInternal which would do mRegistrationInfos.LookupForAdd > + // which will cause a false positive assertion since we're iterating it. > + queue = aRegistrationData->mJobQueues.LookupForAdd(aRegistration->mScope).OrInsert( > + []() { return new ServiceWorkerJobQueue(); }); I don't think this right. Running the unregister from an anonymous job queue means any further jobs that might start for this scope after this won't wait for the unregister to complete.

Comment on attachment 8876255 [details] [diff] [review] part 2 - Optimize hashtable lookups on mRegistrationInfos, mInfos and mJobQueues Review of attachment 8876255 [details] [diff] [review]: ----------------------------------------------------------------- Since this patch interferes with part 3, I'll let you and bkelly talk through how best to deal with this.

OK, I'll leave the troublesome Get+Put as is and skip part 3.

(Filed bug 1372333 on adding a EntryPtr::Remove() method, automatic or not)

Comment on attachment 8876763 [details] [diff] [review] Optimize hashtable lookups on mRegistrationInfos, mInfos and mJobQueues Review of attachment 8876763 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.

Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9b25910374bd part 1 - Use mUpdateTimers.LookupRemoveIf() to avoid a second hashtable lookup for Remove(). Use mUpdateTimers.GetOrInsert() to avoid a second hashtable lookup for Put(). r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/2882dfa38314 part 2 - Optimize hashtable lookups on mRegistrationInfos, mInfos and mJobQueues. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/9b25910374bd https://hg.mozilla.org/mozilla-central/rev/2882dfa38314