Closed
Bug 1370674
Opened 7 years ago
Closed 7 years ago
ServiceWorkerManager does unnecessary hashtable lookups
Categories
(Core :: DOM: Service Workers, enhancement)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(2 files, 3 obsolete files)
4.56 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
3.99 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Updated•7 years ago
|
Blocks: ServiceWorkers-perf
Component: DOM: Workers → DOM: Service Workers
Assignee | ||
Comment 1•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49a9cb1aae9fc8b6cdac377c38ce36428058734d
Attachment #8875463 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•7 years 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•7 years 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•7 years 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•7 years ago
|
||
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•7 years ago
|
Attachment #8875463 -
Attachment is obsolete: true
Assignee | ||
Comment 6•7 years ago
|
||
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•7 years ago
|
||
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)
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=32f9b2a7a9219c8191a9f274f84b6c01d8a325a9
Comment 9•7 years ago
|
||
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 10•7 years ago
|
||
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-
Updated•7 years ago
|
Attachment #8876250 -
Flags: review?(nfroyd) → review+
Comment 11•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
(Filed bug 1372333 on adding a EntryPtr::Remove() method, automatic or not)
Blocks: 1372333
Comment 14•7 years ago
|
||
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•7 years 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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9b25910374bd https://hg.mozilla.org/mozilla-central/rev/2882dfa38314
Status: NEW → RESOLVED
Closed: 7 years 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.
Description
•