413629 - Investigate if parser could be bypassed in some cases when setting .innerHTML

Status: RESOLVED DUPLICATE of bug 922681

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

Description

[Olli Pettay [:smaug][[e-mail]]]

See the "wrong" patch for bug 404386, (attachment 297391 [details] [diff] [review]) and 
https://bugzilla.mozilla.org/show_bug.cgi?id=404386&mark=4,6,7#c4

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

mrbkap, is there some easy way to know if some element needs special handling, 
or other way round, if the element doesn't need special handling?

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Yeah, this is on my list of things to tackle as son as bug 404386 is fixed. I'll probably start on it today.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

bz, I have a patch ready for this, all I'm waiting on is figuring out which elements we want to optimize for. Jst and I looked through the parser code and it looks like we could trivially expose some "tag can accept text" method there, but it looks like there are all sorts of silly elements that would get included that way (like <bgsound>, for instance). Do you think it's worth leaving it up to the parser or should we just hardcode some small-ish (10?) list of common elements?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Is this really a big enough win that:

1)  We want the added complexity (esp. as the HTML5 spec changes)
2)  We want the added cycles in the cases when non-text stuff is being set
   (how much of a hit is this in that case?)

For the rest, I can't answer that question as posed.  To answer it we'd need some in-the-wild data that indicates what the frequency of the various cases is.

Comment 5

[Mike Schroepfer]

I'm not sure it is at this stage of the release.  I'm all for generic optimizations and this is something we can certainly improve later.  But it is not clear we should spend time on this right now.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

I think we should think about this again. Some perf tests do test this;
setting a short value using .innerHTML.

Henri, is there some "easy" way to detect whether a string needs to be
parsed or whether we could just use nsContentUtils::SetNodeTextContent
or something similar?

Comment 7

[Henri Sivonen (:hsivonen)]

(In reply to comment #6)
> Henri, is there some "easy" way to detect whether a string needs to be
> parsed or whether we could just use nsContentUtils::SetNodeTextContent
> or something similar?

I think you could just set text if:
 * The string doesn't contain <, & or U+0000
AND
 * The context node is not one of: html, head, noscript, table, thead, tfoot, tbody, tr, colgroup, frameset, frame, pre, listing or textarea.

In the case of pre, listing and textarea if the string doesn't contain <, & or U+0000, you could delete a leading line feed (if any) and then set the text content.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

Could we do something like this.
Basically opt-in for the commonly used elements.

The patch tries to not regress too badly setting markup in a loop.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a testcase

This is a bit modified testcase from a webkit bug.
(HTML5 parser regressed innerHTML quite a bit there.)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

The patch gives over 50% speed up in that testcase.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 490863 [details] [diff] [review]
WIP

Henri, what do you think about something like this?

Comment 12

[Henri Sivonen (:hsivonen)]

Comment on attachment 490863 [details] [diff] [review]
WIP

This looks OK to me, but I'm a bit uncomfortable about whitelisting 3 elements instead of blacklisting the weird ones. (I realize that the blacklist would be longer and would either involve more pointer compares or a virtual call.)

Comment 13

[Boris Zbarsky [:bzbarsky]]

If we're willing to make nodeinfo structs a bit bigger, we could add a flags field to nodeinfo that would store info about the node type....  In fact this might be worth doing and would allow us to optimize a whole bunch of stuff.  Separate bug?

Or heck, we could use an nsINode flag if we want to make this really fast...

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

We'd need two flags. "Is it ok to bypass parser" and "should delete leading line feed". But I guess we have plenty of bits left atm in nsINode.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Right.  We should do a followup on reorganizing bits; need to think about whether they should all live on Node, or some on Element or what, and whether some should be on nodeinfo or not (more expensive to get those, since it needs an extra fetch).

Comment 16

[Guilherme Lima]

Firefox is a lot faster than competition on the attached testcase.

Nightly - avg 200.35 stdev 5.668112560632508 
Chrome 32 - avg 377.7 stdev 3.451086785347479
IE 11 - avg 1082.85 stdev 19.817353506459937

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I'd say this was fixed in bug 922681.