1452074 - improve the performance related to unresolved custom elements

Status: RESOLVED FIXED

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

Comment 1

[Olli Pettay [:smaug]]

Attachment: testcase

Comment 2

[Olli Pettay [:smaug]]

Attachment: faster_unresolved_ce.diff

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.

Comment 3

[Emilio Cobos Álvarez (: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.

Comment 4

[Olli Pettay [:smaug]]

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/760dcfb2cca4
improve the performance related to unresolved custom elements, r=emilio

Comment 6

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/760dcfb2cca4