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

RESOLVED FIXED in mozilla8

Status

()

Core
DOM
--
enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

(Blocks: 1 bug, {footprint})

Trunk
mozilla8
footprint
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 6 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

6 years ago
I'm working on this.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Depends on: 482909
(Assignee)

Comment 2

6 years ago
Created attachment 516195 [details] [diff] [review]
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.
(Assignee)

Comment 3

6 years ago
The patch applies on top of the patch for bug 482909.
(Assignee)

Comment 4

6 years ago
Created attachment 516555 [details] [diff] [review]
Move fragement parsers to statics in nsContentUtils, v2
Attachment #516195 - Attachment is obsolete: true
Attachment #516555 - Flags: review?(Olli.Pettay)

Comment 5

6 years ago
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.
Attachment #516555 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 6

6 years ago
Created attachment 516591 [details] [diff] [review]
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.
Attachment #516555 - Attachment is obsolete: true
Attachment #516591 - Flags: review?(Olli.Pettay)

Comment 7

6 years ago
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?
(Assignee)

Comment 8

6 years ago
Created attachment 517423 [details] [diff] [review]
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.
Attachment #516591 - Attachment is obsolete: true
Attachment #517423 - Flags: review?(Olli.Pettay)
Attachment #516591 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 9

6 years ago
Created attachment 521481 [details] [diff] [review]
Move fragement parsers to statics in nsContentUtils, v5

Rebasing patch.
Attachment #517423 - Attachment is obsolete: true
Attachment #521481 - Flags: review?(Olli.Pettay)
Attachment #517423 - Flags: review?(Olli.Pettay)
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.
Attachment #517423 - Flags: review+

Updated

6 years ago
Attachment #521481 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 11

6 years ago
Created attachment 522346 [details] [diff] [review]
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.
Attachment #521481 - Attachment is obsolete: true
Attachment #522346 - Flags: review?(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).
Attachment #522346 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 13

6 years ago
(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?
(Assignee)

Comment 14

6 years ago
Created attachment 522659 [details] [diff] [review]
Patch with review comments addressed
Attachment #522346 - Attachment is obsolete: true
> 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.
The other benefit of breaking up patches is that it makes meaningful backouts easier, by the way, and can make some merges easier.
(Assignee)

Updated

6 years ago
Blocks: 563322
Looks like you need to rev nsIDocument's iid.
(Assignee)

Comment 18

6 years ago
(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.
(Assignee)

Updated

6 years ago
Blocks: 563890
(Assignee)

Comment 19

6 years ago
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/449a9666fb5a
Whiteboard: [inbound]
Target Milestone: --- → mozilla8
http://hg.mozilla.org/mozilla-central/rev/449a9666fb5a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.