1861010 - Improve HTML parser attribute name handling a bit.

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, task)

Description

[Emilio Cobos Álvarez (:emilio)]

We do a bit of inefficient stuff, mainly:

• Lots of indirections.
• For non-interned atoms, we end up doing three atom addrefs and releases when we only need one.
• Some redundant hashing.

This seems to improve SP3 performance a bit in the innerHTML heavy bits, see here (though I need to admit I'm not sure what happened with the windows results)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Java part 1

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Java part 2

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1861010 - Avoid some duplicated string hashing in the html parser. r=smaug

I need to check whether we can remove the recently used atom stuff, but this
should be uncontroversial.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1861010 - Improve nsHtml5AttributeName. r=smaug,hsivonen

In particular:

• Store local names, namespaces, etc inline.
• Avoid taking unnecessary atom references for non-interned atoms.

As a follow-up, we can look into making the input data truly static,
which would be nice.

Depends on D191853

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Java part 1 (with minor typo / import fixes)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15de815b169e
Avoid some duplicated string hashing in the html parser. r=smaug

Comment 7

[Henri Sivonen (:hsivonen)]

Comment on attachment 9360209 [details] [diff] [review]
Java part 1 (with minor typo / import fixes)

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

Seems believable. (I'll come back to this.)

::: translator-src/nu/validator/htmlparser/cpptranslate/CppVisitor.java
@@ +1292,5 @@
>              n.getArgs().get(0).accept(this, arg);
>              printer.print(".Release()");
> +        } else if ("addrefIfNonNull".equals(n.getName())
> +                && "Portability".equals(n.getScope().toString())) {
> +            printer.print("NS_IF_ADDREF(");

Please put "NS_IF_ADDREF" and "NS_IF_RELASE" in `CppTypes`. A non-Gecko-flavored target has never emerged, but let's still keep this stuff parametrized there.

Comment 8

[Henri Sivonen (:hsivonen)]

Comment on attachment 9360204 [details] [diff] [review]
Java part 2

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

I trust that this makes sense. rs+

Comment 9

[Henri Sivonen (:hsivonen)]

Comment on attachment 9360209 [details] [diff] [review]
Java part 1 (with minor typo / import fixes)

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

r+ provided that the refcounting macros are parametrized via `CppTypes`.

::: translator-src/nu/validator/htmlparser/cpptranslate/CppVisitor.java
@@ +1292,5 @@
>              n.getArgs().get(0).accept(this, arg);
>              printer.print(".Release()");
> +        } else if ("addrefIfNonNull".equals(n.getName())
> +                && "Portability".equals(n.getScope().toString())) {
> +            printer.print("NS_IF_ADDREF(");

Please put "NS_IF_ADDREF" and "NS_IF_RELASE" in `CppTypes`. A non-Gecko-flavored target has never emerged, but let's still keep this stuff parametrized there.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Attachment: Java part 1 with comments addressed

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/44697a7ca861
Improve nsHtml5AttributeName. r=hsivonen

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1861010 - Avoid leaking on the nsHtml5AttributeName constructor. r=hsivonen

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5ac17201cd79
Avoid leaking on the nsHtml5AttributeName constructor. rpending=hsivonen CLOSED TREE

Comment 14

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/15de815b169e
https://hg.mozilla.org/mozilla-central/rev/44697a7ca861
https://hg.mozilla.org/mozilla-central/rev/5ac17201cd79