Closed
Bug 1452074
Opened 5 years ago
Closed 5 years ago
improve the performance related to unresolved custom elements
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: smaug, Assigned: smaug)
References
Details
Attachments
(2 files)
440 bytes,
text/html
|
Details | |
5.03 KB,
patch
|
emilio
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → bugs
Assignee | ||
Comment 1•5 years ago
|
||
Assignee | ||
Comment 2•5 years ago
|
||
Use hashtable since the array isn't ordered anyhow. CandidateFinder is there for upgrading elements in order. This relies on the fact that if a node has a weak reference, it has always just one such object, stored in the slots. remote: View your change here: remote: https://hg.mozilla.org/try/rev/744d3a0a7948e4d86a007b4405b03c7454a260ed remote: remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=744d3a0a7948e4d86a007b4405b03c7454a260ed remote: recorded changegroup in replication log in 0.079s And no, I don't want to have a typedef for nsTHashtable<nsRefPtrHashKey<nsIWeakReference>. typedef often just decrease readability. Also, CandidateFinder didn't use && for anything, & is good enough.
Assignee | ||
Updated•5 years ago
|
Attachment #8965721 -
Flags: review?(emilio)
Comment 3•5 years ago
|
||
Comment on attachment 8965721 [details] [diff] [review] faster_unresolved_ce.diff Review of attachment 8965721 [details] [diff] [review]: ----------------------------------------------------------------- Looks great, thank you :) ::: dom/base/CustomElementRegistry.cpp @@ +364,5 @@ > void > CustomElementRegistry::UnregisterUnresolvedElement(Element* aElement, > nsAtom* aTypeName) > { > + nsTHashtable<nsRefPtrHashKey<nsIWeakReference>>* candidates = nullptr; I'd say CandidateSet wouldn't hurt readability here, but... :) @@ +582,5 @@ > MOZ_ASSERT(candidates); > CustomElementReactionsStack* reactionsStack = > docGroup->CustomElementReactionsStack(); > > + CandidateFinder finder(*candidates, mWindow->GetExtantDoc()); So, the Move here was clearing the array, wasn't it? I guess the hashtable is now cleared just as a side-effect of Remove(..). That's fine, indeed the move here seemed quite useless.
Attachment #8965721 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 4•5 years ago
|
||
(In reply to Emilio Cobos Álvarez [:emilio] from comment #3) > > + nsTHashtable<nsRefPtrHashKey<nsIWeakReference>>* candidates = nullptr; > > I'd say CandidateSet wouldn't hurt readability here, but... :) To my eyes seeing explicitly what the key type is, is rather valuable. I do agree that with these kinds of nested templates the readability is getting worse. > > @@ +582,5 @@ > > MOZ_ASSERT(candidates); > > CustomElementReactionsStack* reactionsStack = > > docGroup->CustomElementReactionsStack(); > > > > + CandidateFinder finder(*candidates, mWindow->GetExtantDoc()); > > So, the Move here was clearing the array, wasn't it? I guess the hashtable > is now cleared just as a side-effect of Remove(..). That's fine, indeed the > move here seemed quite useless. Remember, Move doesn't move anything. It just gives rvalue.
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/760dcfb2cca4 improve the performance related to unresolved custom elements, r=emilio
Comment 6•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/760dcfb2cca4
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•