366393 - Schedule GC between page loads when possible.

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

This is something we already do to some degree, we currently schedule a 2s timer when we're leaving a page (or closing a window etc) and do the GC off of that timer, which in the real world means that *normally*, at least on a fast connection, the GC that happens when you do normal browsing happens after the page is done loading after clicking on a link. Now that the cycle collector landed (bug 333078) the GCs take a bit more time, and when running tp tests which load page after page after page, we could be better at keeping GC out of the tp times if we schedule them better.

The patch I'm about to attach makes the GC scheduling code take into account whether we're in the middle of loading a page when the 2s GC timer fires, and if we are, it fires another 4s timer and does GC off of that one instead, *unless* the page(s) that were loading when the 2s timer fired finished loading. If a page finishes loading and a secondary GC timer is scheduled, we'll GC right then and cancel the timer.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Better GC scheduling.

Comment 2

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

Comment on attachment 250924 [details] [diff] [review]
Better GC scheduling.

>@@ -3068,18 +3092,69 @@ nsJSContext::Notify(nsITimer *timer)
> {
>   NS_ASSERTION(mContext, "No context in nsJSContext::Notify()!");
> 
>-  // nsCycleCollector_collect() will run a ::JS_GC indirectly,
>-  // so we do not explicitly call ::JS_GC here. 
>-  nsCycleCollector_collect();
>+  NS_RELEASE(sGCTimer);
>+
>+  if (sPendingLoadCount == 0 || sLoadInProgressGCTimer) {
>+    // nsCycleCollector_collect() will run a ::JS_GC() indirectly,
>+    // so we do not explicitly call ::JS_GC() here. 
>+    nsCycleCollector_collect();
>+
>+    sLoadInProgressGCTimer = PR_FALSE;
>+
>+    // Reset sPendingLoadCount in case the timer that fired was a
>+    // timer we scheduled due to a normal GC timer firing while
>+    // documents were loading. If this happens we're waiting for a
>+    // document that is taking a long time to load.
>+    sPendingLoadCount = 0;

The result of this is basically that we ignore loads that take too long, right? Mind adding a comment stating that?

>+nsJSContext::LoadEnd(nsIDOMWindow *aWindow)
>+{
>+  // sPendingLoadCount is not a well managed load counter (and doesn't
>+  // need to be), so make sure we don't make it wrap backwards here.
>+  if (sPendingLoadCount > 0) {
>+    --sPendingLoadCount;
>+  }
>+
>+  nsCOMPtr<nsIDOMWindow> parent;
>+  aWindow->GetParent(getter_AddRefs(parent));
>+
>+  if (parent && parent != aWindow) {
>+    // This is a iframe or other child window load, don't GC at the
>+    // end of child frame loads.
>+    return;
>+  }

I don't think you need this check. If a sub-frame is loaded as part of an outer document then the outer documents load should keep the loadcount from going to 0. And if the user is just loading something inside a frame then we do want to GC once that inner frame is done loading.


>@@ -3105,9 +3182,13 @@ nsJSContext::FireGCTimer()
>   static PRBool first = PR_TRUE;
> 
>   sGCTimer->InitWithCallback(this,
>-                             first ? NS_FIRST_GC_DELAY : NS_GC_DELAY,
>+                             first ? NS_FIRST_GC_DELAY :
>+                             (aLoadInProgress ? NS_LOAD_IN_PROCESS_GC_DELAY :
>+                                                NS_GC_DELAY),
>                              nsITimer::TYPE_ONE_SHOT);

Nit: The precedences for ?: is done such that you can just do

first ? NS_FIRST_GC_DELAY :
aLoadInProgress ? NS_LOAD_IN_PROCESS_GC_DELAY :
NS_GC_DELAY

I.e. it works the same as for |if else-if else|

>@@ -1688,6 +1692,8 @@ DocumentViewerImpl::SetDOMDocument(nsIDOMDocument *aDocument)
>     nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(container);
>     if (window) {
>       window->SetNewDocument(newDoc, nsnull, PR_TRUE);
>+
>+      nsJSContext::LoadStart();
>     }

I don't think you want this call. Would be great if you could check to make sure.

Comment 3

[Johnny Stenback (:jst)]

Attachment: Update to Jonas' comments.

This fixes all the above issues.

Comment 4

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

Comment on attachment 251129 [details] [diff] [review]
Update to Jonas' comments.

Looks good, r/sr=sicking

Comment 5

[Johnny Stenback (:jst)]

Fix checked in.

Comment 6

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

TB shows now new crashes
 @ nsJSContext::LoadEnd  [mozilla\dom\src\base\nsjsenvironment.cpp, line 3194]