959150 - Get the off-the-main-thread machinery out of the way in main-thread-only cases like innerHTML

Status: RESOLVED FIXED

(Core :: DOM: HTML Parser, enhancement)

Description

[Henri Sivonen (:hsivonen)]

Make innerHTML, etc., faster by not having to deal with tree ops.

Comment 1

[Henri Sivonen (:hsivonen)]

Attachment: Part 1: Use void* for elements in the portable parser core

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Part 2: Split stuff out of nsHtml5TreeOperation::Perform

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Part 3: Move stuff that the new methods from part 2 use into a superclass of nsHtml5TreeOpExecutor

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Part 4: Avoid tree ops when parsing with nsHtml5StringParser

Comment 5

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=a758e6b5a620

Still to do: Avoid reallocating nsHtml5HtmlAttributes when working without tree ops.

Also, in principle, nsHtml5AtomTable should be useless in the non-tree op case as well, but most atoms are static atoms anyway, so optimizing the atom stuff doesn't look like a high priority item to me.

Comment 6

[Henri Sivonen (:hsivonen)]

Attachment: Part 3: Move stuff that the new methods from part 2 use into a superclass of nsHtml5TreeOpExecutor, v2 with cycle collection fixed

Comment 7

[Henri Sivonen (:hsivonen)]

Attachment: Part 4: Avoid tree ops when parsing with nsHtml5StringParser, v2 with support for non-unified builds

Comment 8

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=3a4c565aa279

Comment 9

[Henri Sivonen (:hsivonen)]

Attachment: Part 5: Avoid reallocating the attribute holder in the nsHtml5StringParser case

https://tbpl.mozilla.org/?tree=Try&rev=3f183df4b6c0

Comment 10

[Henri Sivonen (:hsivonen)]

Testing with https://bug922018.bugzilla.mozilla.org/attachment.cgi?id=811950 , I'm seeing a 6% improvement.

Comment 11

[Henri Sivonen (:hsivonen)]

https://tbpl.mozilla.org/?tree=Try&rev=b10f4e5e9a82

Comment 12

[Henri Sivonen (:hsivonen)]

Attachment: Part 4: Avoid tree ops when parsing with nsHtml5StringParser, v3 without crashes or memory leaks

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Part 5: Avoid reallocating the attribute holder in the nsHtml5StringParser case, v2 without memory leaks

Comment 14

[Henri Sivonen (:hsivonen)]

This is green on try and works. There are a couple of XXX comments that I will have to resolve in a "part 6". However, claiming a spot in smaug's review queue for the existing parts.

Comment 15

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

Would it be possible to have also a patch which contains all the changes. (such patch isn't perhaps good for reviewing, but is useful to understand the big picture)

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: All-in-one diff

When reading this, I realized I should probably align the attribute deletion code in nsHtml5TreeBuilder.cpp with the attribute deletion code in nsHtml5TreeBuilderCppSupplement.h.

Comment 17

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #16)
> When reading this, I realized I should probably align the attribute deletion
> code in nsHtml5TreeBuilder.cpp with the attribute deletion code in
> nsHtml5TreeBuilderCppSupplement.h.

Never mind. The code is OK, because the TreeBuilder never calls startTag() itself.

Comment 18

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

Comment on attachment 8359200 [details] [diff] [review]
Part 1: Use void* for elements in the portable parser core

In nsHtml5TreeOperation can we assert before 
static_cast<nsIContent**>(some_contentHandler) that we're dealing with right
kind of parser?

Comment 19

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

Comment on attachment 8359721 [details] [diff] [review]
Part 2: Split stuff out of nsHtml5TreeOperation::Perform


>+  aBuilder->HoldElement(newContent);
(I think it would be nice if HoldElement would take already_AddRefed<nsIContent> so that we'd get rid of one extra addref/release. But don't change in this bug.)


>+  if (!aAttributes) {
>+    return newContent.get();
Shouldn't return newContent; work just fine.

>+  }
>+  return newContent.get();
ditto
>+nsIContent*
>+nsHtml5TreeOperation::GetDocumentFragmentForTemplate(nsIContent* aNode)
>+{
>+  dom::HTMLTemplateElement* tempElem =
>+    static_cast<dom::HTMLTemplateElement*>(aNode);
>+  nsRefPtr<dom::DocumentFragment> frag = tempElem->Content();
>+  return frag.get();
Do you need .get()?


>   public:
>+    static inline already_AddRefed<nsIAtom> Reget(nsIAtom* aAtom) {
You just move this, but { should go to the next line.


>+      if (!aAtom || aAtom->IsStaticAtom()) {
>+        return dont_AddRef(aAtom);
>+      }
>+      nsAutoString str;
>+      aAtom->ToString(str);
>+      return do_GetAtom(str);
>+    }
(This all could use a comment. Why on earth do we need to reget the atom? Is that because the original atom is possibly from parser thread, and we want a new one for the main thread?)

Comment 20

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

Comment on attachment 8362959 [details] [diff] [review]
Part 3: Move stuff that the new methods from part 2 use into a superclass of nsHtml5TreeOpExecutor, v2 with cycle collection fixed

I wish we had some tool for make it easier to review code moves :/


>+NS_IMPL_CYCLE_COLLECTION_INHERITED_1(nsHtml5DocumentBuilder, nsContentSink,
>+                                     mOwnedElements)
>+
>+NS_INTERFACE_MAP_BEGIN_CYCLE_COLLECTION_INHERITED(nsHtml5DocumentBuilder)
>+NS_INTERFACE_MAP_END_INHERITING(nsContentSink)
>+

NS_IMPL_ADDREF_INHERITED(nsHtml5DocumentBuilder, nsContentSink)
NS_IMPL_RELEASE_INHERITED(nsHtml5DocumentBuilder, nsContentSink)




>+class nsHtml5DocumentBuilder : public nsContentSink
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTION_CLASS_INHERITED(nsHtml5DocumentBuilder,
>+                                           nsContentSink)
>+
>+  NS_IMETHOD QueryInterface(REFNSIID aIID,                                    \
>+                            void** aInstancePtr);
You want NS_DECL_ISUPPORTS_INHERITED here


>+
>+  inline void HoldElement(nsIContent* aContent) {
{ goes to the next line


>+  inline bool HaveNotified(nsIContent* aNode) {
ditto

>+  void PostPendingAppendNotification(nsIContent* aParent, nsIContent* aChild) {
ditto

>+  void FlushPendingAppendNotifications() {
ditto...and elsewhere

Comment 21

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

Comment on attachment 8365058 [details] [diff] [review]
Part 4: Avoid tree ops when parsing with nsHtml5StringParser, v3 without crashes or memory leaks

I'd like to see an updated patch, hopefully with interdiff, since this diff was super hard to read.

>+  inline nsresult IsBroken() {
{ to the next line, also elsewhere.

>+void
>+nsHtml5OplessBuilder::Start()
>+{
>+  mFlushState = eInFlush;
>+  BeginDocUpdate();
>+}
>+
>+void
>+nsHtml5OplessBuilder::Finish()
>+{
>+  EndDocUpdate();
>+  DropParserAndPerfHint();
>+  mScriptLoader = nullptr;
>+  mDocument = nullptr;
>+  mNodeInfoManager = nullptr;
>+  mCSSLoader = nullptr;
>+  mDocumentURI = nullptr;
>+  mDocShell = nullptr;
>+  mOwnedElements.Clear();
>+  mFlushState = eNotFlushing;
>+}
I'd really wish some comments for how/what nsHtml5OplessBuilder does.



>   if (!aSourceBuffer.IsEmpty()) {
>     bool lastWasCR = false;
>     nsHtml5DependentUTF16Buffer buffer(aSourceBuffer);
>     while (buffer.hasMore()) {
>       buffer.adjust(lastWasCR);
>       lastWasCR = false;
>       if (buffer.hasMore()) {
>         lastWasCR = mTokenizer->tokenizeBuffer(&buffer);
>-        if (mTreeBuilder->HasScript()) {
>-          // If we come here, we are in createContextualFragment() or in the
>-          // upcoming document.parse(). It's unclear if it's really necessary
>-          // to flush here, but let's do so for consistency with other flushes
>-          // to avoid different code paths on the executor side.
>-          mTreeBuilder->Flush(); // Move ops to the executor
>-          mExecutor->FlushDocumentWrite(); // run the ops
>+        if (NS_FAILED(rv = mBuilder->IsBroken())) {
>+          break;
>         }
I don't understand this change. I can understand handling script, but why adding IsBroken()?
(It is hard to follow this patch since it is mostly just moving stuff, but then there are also some other changes.)


> nsHtml5TreeBuilder::appendCharacters(nsIContentHandle* aParent, char16_t* aBuffer, int32_t aStart, int32_t aLength)
> {
>   NS_PRECONDITION(aBuffer, "Null buffer");
>   NS_PRECONDITION(aParent, "Null parent");
> 
>+  if (mBuilder) {
>+    nsresult rv = nsHtml5TreeOperation::AppendText(
>+      aBuffer, // XXX aStart always ignored???
>+      aLength,
>+      static_cast<nsIContent*>(deepTreeSurrogateParent ?
>+                               deepTreeSurrogateParent : aParent),


I really wish nsIContentHandle would be something which holds either
nsIContent* or nsIContent** and it would then have
getters GetContentPtr() and GetContentPtrPtr() 
or some such, and those would assert hard in debug builds if one is trying to access the wrong type.
Is there a reason why nsIContentHandle can't be a real class?


> void
> nsHtml5TreeBuilder::markMalformedIfScript(nsIContentHandle* aElement)
> {
>   NS_PRECONDITION(aElement, "Null element");
> 
>+  if (mBuilder) {
>+    // XXX innerHTML
>+    return;
>+  }
For consistency, shouldn't you QI to script element and SetIsMalformed();?





>+  if (mBuilder) {
>+    nsCOMPtr<nsIAtom> name = nsHtml5TreeOperation::Reget(aName);
>+    nsresult rv = nsHtml5TreeOperation::AppendDoctypeToDocument(
>+      name,
>+      *aPublicId,
>+      *aSystemId,
>+      mBuilder);
I'd write
      nsresult rv =
        nsHtml5TreeOperation::AppendDoctypeToDocument(name,
                                                      *aPublicId,
                                                      *aSystemId,
                                                      mBuilder);



>   if (aNamespace == kNameSpaceID_SVG) {
>+    if (mBuilder) {
>+      // XXX innerHTML
>+      // is this ever needed for the on-the-main-thread case
>+      return;
>+    }
Better to sort that out. Ask jwatt or dholbert?
Do we end up dispatching SVGLoad currently? If so, we should continue doing that and
possibly change that behavior in a followup.

Comment 22

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

Comment on attachment 8365059 [details] [diff] [review]
Part 5: Avoid reallocating the attribute holder in the nsHtml5StringParser case, v2 without memory leaks

>     }
>-    delete attributes;
>+    if (newAttributesEachTime) {
>+      delete attributes;
>+    }
>   } else {
>     if (viewingXmlSource) {
>+      MOZ_ASSERT(newAttributesEachTime);
>       delete attributes;
>     } else {
>       tokenHandler->startTag(tagName, attrs, selfClosing);
>     }
>   }
>   tagName->release();
>   tagName = nullptr;
>-  resetAttributes();
>+  if (newAttributesEachTime) {
>+    attributes = nullptr;
Could you set attributes to nullptr each time you delete it.
That would be less error prone.

>+    bool hasBuilder() {
HasBuilder? (or is this generated from java)

Comment 23

[Henri Sivonen (:hsivonen)]

(In reply to Olli Pettay [:smaug] from comment #18)
> In nsHtml5TreeOperation can we assert before 
> static_cast<nsIContent**>(some_contentHandler) that we're dealing with right
> kind of parser?

In the Init* methods, we  don't have visibility to the fields of the calling nsHtml5TreeBuilder. It would be weird/cluttery to pass the nsHtml5TreeBuilder there only in order to be able to assert stuff about the caller's fields.

Comment 24

[Henri Sivonen (:hsivonen)]

> Do you need .get()?

No. Removing useless use of .get().

> NS_IMPL_ADDREF_INHERITED(nsHtml5DocumentBuilder, nsContentSink)
> NS_IMPL_RELEASE_INHERITED(nsHtml5DocumentBuilder, nsContentSink)

> >+  NS_IMETHOD QueryInterface(REFNSIID aIID,                                    \
> >+                            void** aInstancePtr);
> You want NS_DECL_ISUPPORTS_INHERITED here

Why are these needed? Don't the superclass AddRef/Release methods work just fine here? 

> { goes to the next line

I'm changing these throughout. However, this makes non-generated .h files under parser/html/ self-inconsistent. So far, the style has consistently (within parser/html/) been { on the same line in non-generated .h files and on the next in non-generated .cpp files. When I started developing parser/html/  long ago, I thought the Mozilla style for the placement of the opening brace intentionally differed between .h and .cpp. I'm not sure where I got that idea from.

> I'd really wish some comments for how/what nsHtml5OplessBuilder does.

I'll write some.

> but why adding IsBroken()?

The old code assumed that no failure codes can be seen here. That turns out not to have been really true (see <keygen> on Firefox OS!) and bug 943519 made it intentionally not true. (But I forgot about updating this code when reviewing that change.)

> I really wish nsIContentHandle would be something which holds either
> nsIContent* or nsIContent** and it would then have
> getters GetContentPtr() and GetContentPtrPtr() 
> or some such, and those would assert hard in debug builds if one is trying to access the wrong type.
> Is there a reason why nsIContentHandle can't be a real class?

There doesn't seem to be much practical benefit compared to the current state, since  chances are that if you violated the rule for nsHtml5TreeBuilderCppSupplement.h that nsIContentHandle is nsIContent* when mBuilder is non-null and nsIContent** otherwise, a debug build would crash pretty hard. Also, having assertions for this stuff would not give us full safety anyway, since you could still do wrong things if you don't stay away from tree ops when mBuilder is non-null.

But nsIContentHandle could be a class that wraps a void* and, in debug builds only, a boolean that tracks what the void* is supposed to be.

> For consistency, shouldn't you QI to script element and SetIsMalformed();?
...
> Better to sort that out. Ask jwatt or dholbert?

These are what I referred to in comment 14.

Thanks.

Comment 25

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

(In reply to Henri Sivonen (:hsivonen) from comment #24)

> Why are these needed? Don't the superclass AddRef/Release methods work just
> fine here? 
Sure, but other code tends to use NS_DECL_ISUPPORTS_INHERITED so consistency makes code easier to read.

Comment 26

[Henri Sivonen (:hsivonen)]

Attachment: Part 1: Use void* for elements in the portable parser core, v2

Comment 27

[Henri Sivonen (:hsivonen)]

Attachment: Part 2: Split stuff out of nsHtml5TreeOperation::Perform, v2

Comment 28

[Henri Sivonen (:hsivonen)]

Attachment: Part 3: Move stuff that the new methods from part 2 use into a superclass of nsHtml5TreeOpExecutor, v3

Comment 29

[Henri Sivonen (:hsivonen)]

Attachment: Part 4: Avoid tree ops when parsing with nsHtml5StringParser, v4

Comment 30

[Henri Sivonen (:hsivonen)]

Attachment: Part 5: Avoid reallocating the attribute holder in the nsHtml5StringParser case, v3

Comment 31

[Henri Sivonen (:hsivonen)]

Attachment: Part 6: Fix braces

Comment 32

[Henri Sivonen (:hsivonen)]

Attachment: Part 7: Restore SVG load event and malformed scripts to innerHTML

Comment 33

[Henri Sivonen (:hsivonen)]

Attachment: Part 8: Write a comment for opless document builder

Comment 34

[Henri Sivonen (:hsivonen)]

Attachment: All-in-one diff, v2

I believe I have addressed all review comments except making the void* thing a class. I suggest we get this landed to avoid the whole thing bitrotting during my paternity leave and then I'll make the void* thing into class as a follow-up.

Comment 35

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

Comment on attachment 8385313 [details] [diff] [review]
Part 4: Avoid tree ops when parsing with nsHtml5StringParser, v4


> nsHtml5TreeBuilder::appendChildrenToNewParent(nsIContentHandle* aOldParent, nsIContentHandle* aNewParent)
> {
>   NS_PRECONDITION(aOldParent, "Null old parent");
>   NS_PRECONDITION(aNewParent, "Null new parent");
> 
>+  if (mBuilder) {
>+    nsresult rv = nsHtml5TreeOperation::AppendChildrenToNewParent(
>+      static_cast<nsIContent*>(aOldParent),
>+      static_cast<nsIContent*>(aNewParent),
>+      mBuilder);
>+    if (NS_FAILED_impl(rv)) {
>+      mBuilder->MarkAsBroken(rv);
>+      requestSuspension();
>+    }
Could we not have a helper for MarkAsBroken() + requestSuspension() ?

> nsHtml5TreeBuilder::appendCommentToDocument(char16_t* aBuffer, int32_t aStart, int32_t aLength)
> {
>   NS_PRECONDITION(aBuffer, "Null buffer");
> 
>+  if (mBuilder) {
>+    nsresult rv = nsHtml5TreeOperation::AppendCommentToDocument(
>+      aBuffer, // XXX aStart always ignored???
Does that matter? Can start be something else than 0?
MOZ_ASSERT please.

>   if (aName == nsHtml5Atoms::style || (aNamespace == kNameSpaceID_XHTML && aName == nsHtml5Atoms::link)) {
>+    if (mBuilder) {
>+      mBuilder->FlushPendingAppendNotifications();
>+      mBuilder->UpdateStyleSheet(static_cast<nsIContent*>(aElement));
FlushPendingAppendNotifications can't run scripts, right?
(We seem to have asserts for that).
Otherwise this wouldn't be safe. Flush might kill mBuilder.
Could you MOZ_ASSERT(!nsContentUtils::IsSafeToRunScript())

Comment 36

[Henri Sivonen (:hsivonen)]

Thanks. Landed with the changes requested in comment 35 made and with trivial header fixes to part 7.

https://hg.mozilla.org/integration/mozilla-inbound/rev/261e2d244c54
https://hg.mozilla.org/integration/mozilla-inbound/rev/603b63c33e9a
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0ae8bffb08
https://hg.mozilla.org/integration/mozilla-inbound/rev/14441e528582
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebc67518a962
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6874004efe7
https://hg.mozilla.org/integration/mozilla-inbound/rev/630e489aed30
https://hg.mozilla.org/integration/mozilla-inbound/rev/30bdc9b15e8e

I'll file a follow-up for the making nsIContentHandle into a class tomorrow.

Comment 37

[Henri Sivonen (:hsivonen)]

Try runs for sheriff reference:
https://tbpl.mozilla.org/?tree=Try&rev=775205fed41e
https://tbpl.mozilla.org/?tree=Try&rev=e975441b0a8c

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Backed out for Linux debug bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/e420695e2079

https://tbpl.mozilla.org/php/getParsedLog.php?id=35675531&tree=Mozilla-Inbound

Comment 39

[Henri Sivonen (:hsivonen)]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #38)
> Backed out for Linux debug bustage.

Must have been some sort of unified vs. non-unified build thing.

Re-pushed with a trivial #include change after building linux debug locally with and without unified sources:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4c2bb65c694
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec506a5c424c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8885a39733df
https://hg.mozilla.org/integration/mozilla-inbound/rev/5acef36769eb
https://hg.mozilla.org/integration/mozilla-inbound/rev/aeef9a68e4a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5182bb4118cc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f51632702297
https://hg.mozilla.org/integration/mozilla-inbound/rev/47882484a98d

Comment 40

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/projects/htmlparser/rev/e015937fefc7

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e4c2bb65c694
https://hg.mozilla.org/mozilla-central/rev/ec506a5c424c
https://hg.mozilla.org/mozilla-central/rev/8885a39733df
https://hg.mozilla.org/mozilla-central/rev/5acef36769eb
https://hg.mozilla.org/mozilla-central/rev/aeef9a68e4a5
https://hg.mozilla.org/mozilla-central/rev/5182bb4118cc
https://hg.mozilla.org/mozilla-central/rev/f51632702297
https://hg.mozilla.org/mozilla-central/rev/47882484a98d