1370674 - ServiceWorkerManager does unnecessary hashtable lookups

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement)

Comment 1

[Mats Palmgren (inactive)]

Attachment: Use mUpdateTimers.LookupRemoveIf() to avoid a second hashtable lookup for Remove(). Use mUpdateTimers.GetOrInsert() to avoid a second hashtable lookup for Put()

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a9cb1aae9fc8b6cdac377c38ce36428058734d

Comment 2

[Mats Palmgren (inactive)]

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

Comment 3

[Mats Palmgren (inactive)]

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 4

[Mats Palmgren (inactive)]

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>& ...

Comment 5

[Mats Palmgren (inactive)]

Attachment: part 1 - Use mUpdateTimers.LookupRemoveIf() to avoid a second hashtable lookup for Remove(). Use mUpdateTimers.GetOrInsert() to avoid a second hashtable lookup for Put()

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.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 2 - Optimize hashtable lookups on mRegistrationInfos, mInfos and mJobQueues

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

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 3 - Refactor Unregister/ForceUnregister to avoid unnecessary hashtable lookups and string copying.

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 8

[Mats Palmgren (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32f9b2a7a9219c8191a9f274f84b6c01d8a325a9

Comment 9

[Nathan Froyd [:froydnj]]

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 10

[Ben Kelly [:bkelly, not reviewing]]

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 11

[Nathan Froyd [:froydnj]]

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.

Comment 12

[Mats Palmgren (inactive)]

Attachment: Optimize hashtable lookups on mRegistrationInfos, mInfos and mJobQueues

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

Comment 13

[Mats Palmgren (inactive)]

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

Comment 14

[Nathan Froyd [:froydnj]]

Comment on attachment 8876763 [details] [diff] [review]
Optimize hashtable lookups on mRegistrationInfos, mInfos and mJobQueues

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

Thanks.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Carsten Book [:Tomcat]]

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