Closed Bug 959150 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: HTML Parser, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: hsivonen, Assigned: hsivonen)

References

(Depends on 1 open bug)

Details

Attachments

(9 files, 10 obsolete files)

78.23 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
43.68 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
34.16 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
73.49 KB, patch
smaug
: review+
Details | Diff | Splinter Review
16.21 KB, patch
hsivonen
: review+
Details | Diff | Splinter Review
34.99 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.70 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.14 KB, patch
smaug
: review+
Details | Diff | Splinter Review
222.00 KB, patch
Details | Diff | Splinter Review
Make innerHTML, etc., faster by not having to deal with tree ops.
Blocks: 922018
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.
Attachment #8362960 - Attachment description: Avoid tree ops when parsing with nsHtml5StringParser, v2 with support for non-unified builds → Part 4: Avoid tree ops when parsing with nsHtml5StringParser, v2 with support for non-unified builds
Testing with https://bug922018.bugzilla.mozilla.org/attachment.cgi?id=811950 , I'm seeing a 6% improvement.
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.
Attachment #8359200 - Flags: review?(bugs)
Attachment #8359721 - Flags: review?(bugs)
Attachment #8362959 - Flags: review?(bugs)
Attachment #8365058 - Flags: review?(bugs)
Attachment #8365059 - Flags: review?(bugs)
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)
Attached patch All-in-one diff (obsolete) — Splinter Review
When reading this, I realized I should probably align the attribute deletion code in nsHtml5TreeBuilder.cpp with the attribute deletion code in nsHtml5TreeBuilderCppSupplement.h.
(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 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?
Attachment #8359200 - Flags: review?(bugs) → review+
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?)
Attachment #8359721 - Flags: review?(bugs) → review+
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
Attachment #8362959 - Flags: review?(bugs) → review+
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.
Attachment #8365058 - Flags: review?(bugs) → review-
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)
Attachment #8365059 - Flags: review?(bugs) → review+
(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.
> 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.
(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.
Attachment #8385316 - Flags: review?(bugs)
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.
Attachment #8365507 - Attachment is obsolete: true
Attachment #8385317 - Flags: review?(bugs) → review+
Attachment #8385319 - Flags: review?(bugs) → review+
Attachment #8385316 - Flags: review?(bugs) → review+
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())
Attachment #8385313 - Flags: review?(bugs) → review+
Depends on: 980317
Depends on: 981279
You need to log in before you can comment on or make changes to this bug.