596182 - Should cache fragment parsers on a per-process basis instead of per-document

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Currently, Gecko caches fragment parsers on a per-document bases.

Since fragment parsing always runs on the main thread, runs to completion and can't be invoked re-entrantly, we could save some memory by making each process have one HTML fragment parser and one XML fragment parser shared by all the documents in that process.

Comment 1

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

I'm working on this.

Comment 2

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Attachment: Move fragement parsers to statics in nsContentUtils

This patch
 * moves fragment parsers to statics in nsContentUtils
 * makes fragment parsing not care about the HTML5 parser pref (the rest of the pref is going to go away in my upcoming patches anyway)
 * removes resulting dead code
 * makes fragment parsers not observe sheets (done in the same patch to avoid crashes)

Non-goals for this patch:
 * Disentangling fragment parsers from normal parsers.
 * Getting rid of useless footprint on fragment parsers themselves.
 * Making the XML parser APIs nicer.

Comment 3

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

The patch applies on top of the patch for bug 482909.

Comment 4

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Attachment: Move fragement parsers to statics in nsContentUtils, v2

Comment 5

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

Comment on attachment 516555 [details] [diff] [review]
Move fragement parsers to statics in nsContentUtils, v2


>+class nsContentUtilsFragmentParserDropper : public nsIObserver
>+{
Perhaps LayoutShutdownObserver could call a method in nsContentUtils,
say XPCOMShutdown() and then call DropFragmentParsers in that method.



>+// static
>+void
>+nsContentUtils::PrepareFragmentParserDropper()
>+{
>+  if (sFragmentParserDropperPrepared) {
>+    return;
>+  }
>+  sFragmentParserDropperPrepared = PR_TRUE;
>+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>+  NS_ASSERTION(os, "do_GetService failed");
>+  os->AddObserver(new nsContentUtilsFragmentParserDropper(),
>+                  "xpcom-shutdown",
>+                  PR_FALSE);
>+}
>+
>+void
>+nsContentUtils::DropFragmentParsers()
>+{
>+  NS_IF_RELEASE(sHTMLFragmentParser);
>+  NS_IF_RELEASE(sXMLFragmentParser);
>+  NS_IF_RELEASE(sXMLFragmentSink);
>+}
>+
>+/* static */
>+void
>+nsContentUtils::ParseFragmentHTML(const nsAString& aSourceBuffer,
>+                                  nsIContent* aTargetNode,
>+                                  nsIAtom* aContextLocalName,
>+                                  PRInt32 aContextNamespace,
>+                                  PRBool aQuirks,
>+                                  PRBool aPreventScriptExecution)
>+{
>+  if (!sHTMLFragmentParser) {
>+    nsCOMPtr<nsIParser> parser = nsHtml5Module::NewHtml5Parser();
>+    nsRefPtr<nsAHtml5FragmentParser> fragmentParser =
>+        static_cast<nsAHtml5FragmentParser*>(parser.get());
>+    fragmentParser.forget(&sHTMLFragmentParser);

Maybe 
sHTMLFragmentParser =
  static_cast<nsAHtml5FragmentParser*>(nsHtml5Module::NewHtml5Parser().get());


>+  if (!sXMLFragmentSink) {
>+    nsCOMPtr<nsIFragmentContentSink> sink;
>+    NS_NewXMLFragmentContentSink(getter_AddRefs(sink));
>+    sink.forget(&sXMLFragmentSink);
>+    // sXMLFragmentSink now owns the sink
>+  }
Why not
NS_NewXMLFragmentContentSink(&sXMLFragmentSink);


>+
>+  nsresult rv =
>+      sXMLFragmentParser->ParseFragment(aSourceBuffer,
>+                                        aTagStack);
some extra spaces before sXMLFragmentParser



>+nsXMLFragmentContentSink::GetFragment(nsIDOMDocumentFragment** aFragment)
> {
>   *aFragment = nsnull;
>+  mTargetDocument = nsnull;
>+  mNodeInfoManager = nsnull;
>+  mScriptLoader = nsnull;
>+  mCSSLoader = nsnull;
>+  mContentStack.Clear();
>+  mDocumentURI = nsnull;
>+  mDocShell = nsnull;
>   if (mParseError) {
>     //XXX PARSE_ERR from DOM3 Load and Save would be more appropriate
>+    mRoot = nsnull;
>+    mParseError = PR_FALSE;
>     return NS_ERROR_DOM_SYNTAX_ERR;
>   } else if (mRoot) {
>     nsresult rv = CallQueryInterface(mRoot, aFragment);
>-    if (NS_SUCCEEDED(rv) && aWillOwnFragment) {
>-      mRoot = nsnull;
>-    }
>+    mRoot = nsnull;
>     return rv;
>   } else {
>     return NS_OK;
>   }
> }
I don't understand this. Why does a getter method set seemingly random member variables to null?
After the changes it sounds like the method should be called TakeFragment() or some such.

Comment 6

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Attachment: Move fragement parsers to statics in nsContentUtils, v3

(In reply to comment #5)
> Comment on attachment 516555 [details] [diff] [review]
> Move fragement parsers to statics in nsContentUtils, v2
> 
> 
> >+class nsContentUtilsFragmentParserDropper : public nsIObserver
> >+{
> Perhaps LayoutShutdownObserver could call a method in nsContentUtils,
> say XPCOMShutdown() and then call DropFragmentParsers in that method.

Done.

> >+// static
> >+void
> >+nsContentUtils::PrepareFragmentParserDropper()
> >+{
> >+  if (sFragmentParserDropperPrepared) {
> >+    return;
> >+  }
> >+  sFragmentParserDropperPrepared = PR_TRUE;
> >+  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> >+  NS_ASSERTION(os, "do_GetService failed");
> >+  os->AddObserver(new nsContentUtilsFragmentParserDropper(),
> >+                  "xpcom-shutdown",
> >+                  PR_FALSE);
> >+}
> >+
> >+void
> >+nsContentUtils::DropFragmentParsers()
> >+{
> >+  NS_IF_RELEASE(sHTMLFragmentParser);
> >+  NS_IF_RELEASE(sXMLFragmentParser);
> >+  NS_IF_RELEASE(sXMLFragmentSink);
> >+}
> >+
> >+/* static */
> >+void
> >+nsContentUtils::ParseFragmentHTML(const nsAString& aSourceBuffer,
> >+                                  nsIContent* aTargetNode,
> >+                                  nsIAtom* aContextLocalName,
> >+                                  PRInt32 aContextNamespace,
> >+                                  PRBool aQuirks,
> >+                                  PRBool aPreventScriptExecution)
> >+{
> >+  if (!sHTMLFragmentParser) {
> >+    nsCOMPtr<nsIParser> parser = nsHtml5Module::NewHtml5Parser();
> >+    nsRefPtr<nsAHtml5FragmentParser> fragmentParser =
> >+        static_cast<nsAHtml5FragmentParser*>(parser.get());
> >+    fragmentParser.forget(&sHTMLFragmentParser);
> 
> Maybe 
> sHTMLFragmentParser =
>   static_cast<nsAHtml5FragmentParser*>(nsHtml5Module::NewHtml5Parser().get());

Changed.

> >+  if (!sXMLFragmentSink) {
> >+    nsCOMPtr<nsIFragmentContentSink> sink;
> >+    NS_NewXMLFragmentContentSink(getter_AddRefs(sink));
> >+    sink.forget(&sXMLFragmentSink);
> >+    // sXMLFragmentSink now owns the sink
> >+  }
> Why not
> NS_NewXMLFragmentContentSink(&sXMLFragmentSink);

Changed.

> >+
> >+  nsresult rv =
> >+      sXMLFragmentParser->ParseFragment(aSourceBuffer,
> >+                                        aTagStack);
> some extra spaces before sXMLFragmentParser

Two spaces removed.

> >+nsXMLFragmentContentSink::GetFragment(nsIDOMDocumentFragment** aFragment)
> > {
> >   *aFragment = nsnull;
> >+  mTargetDocument = nsnull;
> >+  mNodeInfoManager = nsnull;
> >+  mScriptLoader = nsnull;
> >+  mCSSLoader = nsnull;
> >+  mContentStack.Clear();
> >+  mDocumentURI = nsnull;
> >+  mDocShell = nsnull;
> >   if (mParseError) {
> >     //XXX PARSE_ERR from DOM3 Load and Save would be more appropriate
> >+    mRoot = nsnull;
> >+    mParseError = PR_FALSE;
> >     return NS_ERROR_DOM_SYNTAX_ERR;
> >   } else if (mRoot) {
> >     nsresult rv = CallQueryInterface(mRoot, aFragment);
> >-    if (NS_SUCCEEDED(rv) && aWillOwnFragment) {
> >-      mRoot = nsnull;
> >-    }
> >+    mRoot = nsnull;
> >     return rv;
> >   } else {
> >     return NS_OK;
> >   }
> > }
> I don't understand this. Why does a getter method set seemingly random member
> variables to null?
> After the changes it sounds like the method should be called TakeFragment() or
> some such.

Even before, the method has de facto been a method that must be called exactly once when finishing fragment parsing. Renamed to reflect that.

The members are set to null to avoid retaining them live in memory after the parse when the parser and sink stay alive for reuse.

Comment 7

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

Comment on attachment 516591 [details] [diff] [review]
Move fragement parsers to statics in nsContentUtils, v3


>   /**
>+   * Invoke the fragment parsing algorithm (innerHTML) using the HTML parser.
>+   *
>+   * @param aSourceBuffer the string being set as innerHTML
>+   * @param aTargetNode the target container
>+   * @param aContextLocalName local name of context node
>+   * @param aContextNamespace namespace of context node
>+   * @param aQuirks true to make <table> not close <p>
>+   * @param aPreventScriptExecution true to prevent scripts from executing;
>+   * don't set to false when parsing into a target node that has been bound
>+   * to tree.
'don't...' could start under aPreventScriptExecution. Would we easier to read that way.


>   rv = mCSSLoader->LoadStyleLink(aElement, url, aTitle, aMedia, aAlternate,
>-                                 this, &isAlternate);
>+                                 mFragmentMode ? nsnull : this, &isAlternate);
I don't know what these changes are. In comment 2 you talk about crashes. What crashes?
Do we have tests?

Comment 8

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Attachment: Move fragement parsers to statics in nsContentUtils, v4

(In reply to comment #7)
> >   /**
> >+   * Invoke the fragment parsing algorithm (innerHTML) using the HTML parser.
> >+   *
> >+   * @param aSourceBuffer the string being set as innerHTML
> >+   * @param aTargetNode the target container
> >+   * @param aContextLocalName local name of context node
> >+   * @param aContextNamespace namespace of context node
> >+   * @param aQuirks true to make <table> not close <p>
> >+   * @param aPreventScriptExecution true to prevent scripts from executing;
> >+   * don't set to false when parsing into a target node that has been bound
> >+   * to tree.
> 'don't...' could start under aPreventScriptExecution. Would we easier to read
> that way.

Changed in this new attachment.

> >   rv = mCSSLoader->LoadStyleLink(aElement, url, aTitle, aMedia, aAlternate,
> >-                                 this, &isAlternate);
> >+                                 mFragmentMode ? nsnull : this, &isAlternate);
> I don't know what these changes are. In comment 2 you talk about crashes. What
> crashes?

If the content sink observes style sheet loads but the sink becomes disassociated from the script loader before the sheet loads completes, when a sheet load finishes, the sink tries to call a method on null script loader and crashes.

Having the sink observe style sheet loads in the fragment case is a bug anyway. See 631547 comment 2.

> Do we have tests?

We have tests that don't test this in particular but that crash without these tweaks once the fragment parsers no longer live with a document for the duration of style sheet loads. So in that sense, we have tests.

Comment 9

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Attachment: Move fragement parsers to statics in nsContentUtils, v5

Rebasing patch.

Comment 10

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

Comment on attachment 517423 [details] [diff] [review]
Move fragement parsers to statics in nsContentUtils, v4


>+nsContentUtils::ParseFragmentXML(const nsAString& aSourceBuffer,
>+                                 nsIDocument* aDocument,
>+                                 nsTArray<nsString>& aTagStack,
>+                                 nsIDOMDocumentFragment** aReturn)
>+{
>+  if (!sXMLFragmentParser) {
>+    nsresult rv;
>+    nsCOMPtr<nsIParser> parser = do_CreateInstance(kCParserCID, &rv);
No need for rv




r=me, but I think this needs a second review.

Comment 11

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Attachment: Move fragement parsers to statics in nsContentUtils, v6

(In reply to comment #10)
> Comment on attachment 517423 [details] [diff] [review]
> Move fragement parsers to statics in nsContentUtils, v4
> 
> 
> >+nsContentUtils::ParseFragmentXML(const nsAString& aSourceBuffer,
> >+                                 nsIDocument* aDocument,
> >+                                 nsTArray<nsString>& aTagStack,
> >+                                 nsIDOMDocumentFragment** aReturn)
> >+{
> >+  if (!sXMLFragmentParser) {
> >+    nsresult rv;
> >+    nsCOMPtr<nsIParser> parser = do_CreateInstance(kCParserCID, &rv);
> No need for rv

Removed.

> r=me, but I think this needs a second review.

Thank you. Requesting a second review from bz.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 522346 [details] [diff] [review]
Move fragement parsers to statics in nsContentUtils, v6

>+  // If this is a fragment parser and we don't want
>+  // to observe.

s/and/then/ and this should probably all be on one line.

>         !(nsGkAtoms::html == contextAsContent->Tag() &&
>           contextAsContent->IsHTML())) {

This is preexisting, but how about:

  !contextAsContent->IsHTML(nsGkAtoms::html)

?

r=me, but for future reference this would have been way easier to review as 3 or more patches (first patch to factor out the nsParserConstants stuff, second patch for the substantive changes, third patch for the dead-code removal).

Comment 13

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

(In reply to comment #12)
> r=me,

Thanks.

> but for future reference this would have been way easier to review as 3
> or more patches (first patch to factor out the nsParserConstants stuff, second
> patch for the substantive changes, third patch for the dead-code removal).

Yeah... The problem was I didn't write the patch is such a coherent order.

Do I need sr from a third person for this?

Comment 14

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Attachment: Patch with review comments addressed

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Yeah... The problem was I didn't write the patch is such a coherent order.

hg qrecord for the win?  ;)

I don't think this needs more review.

Comment 16

[Boris Zbarsky [:bzbarsky]]

The other benefit of breaking up patches is that it makes meaningful backouts easier, by the way, and can make some merges easier.

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Looks like you need to rev nsIDocument's iid.

Comment 18

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

(In reply to comment #17)
> Looks like you need to rev nsIDocument's iid.

Good catch. Thanks. I'll land with a revved iid once my queue unblocks.

Comment 19

[Henri Sivonen (:hsivonen) (not reading bugmail while on leave)]

Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/449a9666fb5a

Comment 20

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/449a9666fb5a