ServiceWorkerManager does unnecessary hashtable lookups

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mats, Assigned: mats)

Tracking

(Blocks: 2 bugs, {perf})

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
Blocks: 1328622
Component: DOM: Workers → DOM: Service Workers
(Assignee)

Comment 1

a year ago
Created 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()

https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a9cb1aae9fc8b6cdac377c38ce36428058734d
Attachment #8875463 - Flags: review?(nfroyd)
(Assignee)

Comment 2

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

Comment 3

a year ago
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.
(Assignee)

Comment 4

a year ago
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>& ...
Attachment #8875463 - Flags: review?(nfroyd)
(Assignee)

Comment 5

a year ago
Created attachment 8876250 [details] [diff] [review]
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.
Attachment #8876250 - Flags: review?(nfroyd)
(Assignee)

Updated

a year ago
Attachment #8875463 - Attachment is obsolete: true
(Assignee)

Comment 6

a year ago
Created attachment 8876255 [details] [diff] [review]
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...
Attachment #8876255 - Flags: review?(nfroyd)
(Assignee)

Comment 7

a year ago
Created attachment 8876259 [details] [diff] [review]
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.
Attachment #8876259 - Flags: review?(nfroyd)
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.
Attachment #8876259 - Flags: review?(nfroyd) → review?(bkelly)
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.
Attachment #8876259 - Flags: review?(bkelly) → review-
Attachment #8876250 - Flags: review?(nfroyd) → review+
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.
Attachment #8876255 - Flags: review?(nfroyd)
(Assignee)

Comment 12

a year ago
Created attachment 8876763 [details] [diff] [review]
Optimize hashtable lookups on mRegistrationInfos, mInfos and mJobQueues

OK, I'll leave the troublesome Get+Put as is and skip part 3.
Attachment #8876255 - Attachment is obsolete: true
Attachment #8876259 - Attachment is obsolete: true
Attachment #8876763 - Flags: review?(nfroyd)
(Assignee)

Comment 13

a year ago
(Filed bug 1372333 on adding a EntryPtr::Remove() method, automatic or not)
Blocks: 1372333
Comment on attachment 8876763 [details] [diff] [review]
Optimize hashtable lookups on mRegistrationInfos, mInfos and mJobQueues

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

Thanks.
Attachment #8876763 - Flags: review?(nfroyd) → review+

Comment 15

a year ago
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

Comment 16

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b25910374bd
https://hg.mozilla.org/mozilla-central/rev/2882dfa38314
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.