562013 - Consider caching the documentFragment for innerHTML, or at least cache the nodeinfo, or parse directly to the destination node

Status: RESOLVED FIXED

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

Description

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

This shows up in the profiles. Pretty high even with .innerHTML = "some simple string"

Caching documentFragment safely can be tricky.

Comment 1

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

Or actually, if there are no mutation event listeners, caching of document fragment should be doable.

Comment 2

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

Attachment: Something like this

The patch is still a bit ugly.
There shouldn't be any need to add the extra QI.

But even this seems to cut 5% of https://bugzilla.mozilla.org/attachment.cgi?id=289857 (which tests setting plain text to innerHTML)

Comment 3

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

Attachment: a test (innerHTML = "some hml, not only plain text")

Comment 4

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

Attachment: a bit better patch

With the patch testcase runs faster in Gecko than in trunk webkit.

https://bugzilla.mozilla.org/attachment.cgi?id=289857 is still faster in
Webkit, but it cheats and works against the spec.

The patch could be probably a bit simpler if html5 was enabled always.

Comment 5

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

And we should eventually convert all the callers to use nsINode, never
nsIDOMXXX.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'd say simplify and always create a docfragment when we're creating a document.

Though ideally I think we should parse directly into the target node, and avoid using a document fragment altogether. That should also save us a bunch of calls to BindToTree/UnbindFromTree.

Might be easier to do only for the HTML5 parser. (and fall back to a slower path when the html5 parser isn't enabled for now, until we kill the old parser)

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If you do go for the parse-directly-to-the-destination path, then just fire mutation events manually after the parsing. The same way that the insert-doc-fragment code does.

Comment 8

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

Parsing directly to target node might be a good option for html5
but mutation event handling would be a bit ugly.
So for now using fragment could be ok.

Comment 9

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

Or Henri, what do you think about parsing directly to the destination?

Comment 10

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

(Btw, seems like fragment's mutation event handling doesn't dispatch
events in all the cases when it should :( )

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Given that mutation events is something that should be phased out, I don't think we should architect too much of our code around their existence.

Comment 12

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

which again would be in favor of using document fragment.
I'd rather not write yet another case for dispatching mutation events.
Document fragment got it already wrong.

Comment 13

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

...which I'll fix.

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to comment #9)
> Or Henri, what do you think about parsing directly to the destination?

I already considered it. See the thread starting at
http://lists.w3.org/Archives/Public/public-html/2008Nov/0192.html

However, I decided not to try anything new in order to avoid having to emulate the right mutation events. (The form pointer and <base> also came up, but I'm not sure if those would still be actual problems.)

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think what's needed is just making sure that we don't bind controls that aren't inside a <form> to the last <form>. Instead they should be bound to any ancestor <form> of the node which we're setting .innerHTML on.

As for the mutation events, they seem like a silly reason to not do this perf optimization. We can likely share the code with the fragment-insertion code (once that's fixed). Eventually that code should go away anyway.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

<base> shouldn't be a problem any more now that it's self-managed and doesn't require any content sink support.

Comment 17

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

I'm not going to implement the parse-directly-to-the-destination in this bug.

Comment 18

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

hmm, or let me re-think. The current setup is nicer, but slower...

Comment 19

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

Attachment: Don't use document fragment at all with html5 parser

This depends on bug 562652.
Needs still some testing.

Comment 20

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

Comment on attachment 442401 [details] [diff] [review]
Don't use document fragment at all with html5 parser

Henri, would this be all wrong?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yay!!

This doesn't deal with ensuring that orphaned <input>s end up in the correct <form>, does it? I.e. for markup like

<form id=A>
  <div id=target></div>
</form>

$("target").innerHTML = "<p><form id=B></p><input id=orphan>"
is($("orphan").form, $("A"))

should pass

Comment 22

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

Attachment: form testcase

HTML5 parser changes behavior in testcase anyway.
The patch in this bug keeps the existing html5 parser behavior.

Comment 23

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

Attachment: er, this is the form test

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Oh, the <form> closes the <p> in the HTML5 parser. The test needs to be

<form id=A>
  <div id=target></div>
</form>

$("target").innerHTML = "<div><form id=B></div><input id=orphan>"
is($("orphan").form, $("A"))


Or possibly even

$("target").innerHTML = "<table><tr><td><form id=B></table><input id=orphan>"

Comment 25

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

Attachment: This breaks

I have no idea how to fix this case. I'd rather not hack the parser.

Comment 26

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

Or, I don't understand  why the parser or something else does what it does.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

What is needed is to set a flag in the parser not perform the steps here:

http://mxr.mozilla.org/mozilla-central/source/parser/html/nsHtml5TreeOperation.cpp#453

Comment 28

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

Henri, could you perhaps tell how to do that? Or perhaps even provide the patch.

Comment 29

[Henri Sivonen (:hsivonen)]

Comment on attachment 442401 [details] [diff] [review]
Don't use document fragment at all with html5 parser

>   // Remove childnodes
>   nsContentUtils::SetNodeTextContent(this, EmptyString(), PR_FALSE);
[...]
>+    PRInt32 oldChildCount = GetChildCount();

Isn't oldChildCount always zero here? Or more to the point, if it isn't zero, isn't that an at least assertion-worthy problem?

>   if (scripts_enabled) {
>     // If we disabled scripts, re-enable them now that we're
>     // done. Don't fire JS timeouts when enabling the context here.
> 
>     loader->SetEnabled(PR_TRUE);
>   }

Is this script disabling and re-enabling for <script> elements or also for something else? The HTML5 parser in the fragment mode already takes care of marking <script>s already executed.

feedback+ suggesting those points to be reconsidered.

(In reply to comment #28)
> Henri, could you perhaps tell how to do that? Or perhaps even provide the
> patch.

I can provide a patch if we first figure out if we want to make the parser not associate elements with a form ever in the fragment case (including createContextualFragment on ranges) or if we want to special-case the innerHTML setter only. Do we care about supporting the case where someone calls createContextualFragment with a malformed form and proceeds to submit the form directly from the fragment before inserting it elsewhere first?

Comment 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> I can provide a patch if we first figure out if we want to make the parser not
> associate elements with a form ever in the fragment case (including
> innerHTML createContextualFragment on ranges) or if we want to special-case
> the setter only. Do we care about supporting the case where someone calls
> createContextualFragment with a malformed form and proceeds to submit the form
> directly from the fragment before inserting it elsewhere first?

I don't think it's terribly important if we special case .innerHTML, or if we do it for all fragment parsing. Simplicity seems like the more important goal. Until proven otherwise, I think it's safe to guess that the web doesn't depend on any particular behavior for createContextualFragment.

However all else being equal, I would say that the explicit form-binding thing is fairly evil, and we should limit it as much as possible.

Comment 31

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

(In reply to comment #29)
> Isn't oldChildCount always zero here? Or more to the point, if it isn't zero,
> isn't that an at least assertion-worthy problem?

Because of mutation events, oldChildCount may not be zero and IMO
there isn't anything to assert because we handle the case.

I don't change scriptloader handling in the patch, but if there is something
to change, I could file a followup bug.





> 
> >   if (scripts_enabled) {
> >     // If we disabled scripts, re-enable them now that we're
> >     // done. Don't fire JS timeouts when enabling the context here.
> > 
> >     loader->SetEnabled(PR_TRUE);
> >   }
> 
> Is this script disabling and re-enabling for <script> elements or also for
> something else? The HTML5 parser in the fragment mode already takes care of
> marking <script>s already executed.
> 
> feedback+ suggesting those points to be reconsidered.
> 
> (In reply to comment #28)
> > Henri, could you perhaps tell how to do that? Or perhaps even provide the
> > patch.
> 
> I can provide a patch if we first figure out if we want to make the parser not
> associate elements with a form ever in the fragment case (including
> createContextualFragment on ranges) or if we want to special-case the innerHTML
> setter only. Do we care about supporting the case where someone calls
> createContextualFragment with a malformed form and proceeds to submit the form
> directly from the fragment before inserting it elsewhere first?

Comment 32

[Henri Sivonen (:hsivonen)]

Attachment: Make the parser not associate elements with the form pointer in the fragment mode

(In reply to comment #30)
> > I can provide a patch if we first figure out if we want to make the parser not
> > associate elements with a form ever in the fragment case (including
> > innerHTML createContextualFragment on ranges) or if we want to special-case
> > the setter only. Do we care about supporting the case where someone calls
> > createContextualFragment with a malformed form and proceeds to submit the form
> > directly from the fragment before inserting it elsewhere first?
> 
> I don't think it's terribly important if we special case .innerHTML, or if we
> do it for all fragment parsing. Simplicity seems like the more important goal.
> Until proven otherwise, I think it's safe to guess that the web doesn't depend
> on any particular behavior for createContextualFragment.
> 
> However all else being equal, I would say that the explicit form-binding thing
> is fairly evil, and we should limit it as much as possible.

OK. I've attached a patch that makes the fragment case in the parser core not associate elements with the form pointer. (I'm assuming that we'll request this change in the spec, so I've put the change in the cross-language part of the parser instead of putting it into the Gecko integration layer.)

(In reply to comment #31)
> (In reply to comment #29)
> > Isn't oldChildCount always zero here? Or more to the point, if it isn't zero,
> > isn't that an at least assertion-worthy problem?
> 
> Because of mutation events, oldChildCount may not be zero and IMO
> there isn't anything to assert because we handle the case.

OK.

> I don't change scriptloader handling in the patch, but if there is something
> to change, I could file a followup bug.

I think it's worthwhile to investigate, as a follow-up, if the scriptloader part can be optimized away.

Comment 33

[Henri Sivonen (:hsivonen)]

The patch doesn't handle Document.innerHTML = "foo" case, since that case doesn't have a context node, but then this bug isn't about optimizing that case anyway.

Comment 34

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

There is document.innerHTML? Gecko doesn't support that.
I'll file a new bug for that.

Comment 35

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

Attachment: both patches

I'll push this once it has passed tests on tryserver

Comment 36

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

Attachment: without XML parsing error reporting problem

Comment 37

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

http://hg.mozilla.org/mozilla-central/rev/d660bc26395e

Comment 38

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

This is great stuff -- will it show up on any of our existing benchmarks, or should we add something to track?

Comment 39

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

Dromaeo tests some innerHTML, though just activating html5 should have given
significant speed up there.

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For what it's worth, mutation events should no longer be a concern for parsing directly to the destination. As long as a update batch spans the whole function, all mutation events will be delayed until that update batch is ended.