Closed Bug 1452074 Opened 2 years ago Closed 2 years ago

improve the performance related to unresolved custom elements

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: smaug, Assigned: smaug)

References

Details

Attachments

(2 files)

No description provided.
Assignee: nobody → bugs
Attached file testcase
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.
Attachment #8965721 - Flags: review?(emilio)
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+
(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
Depends on: 1452386
https://hg.mozilla.org/mozilla-central/rev/760dcfb2cca4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Duplicate of this bug: 1414178
You need to log in before you can comment on or make changes to this bug.