Schedule GC between page loads when possible.

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: jst, Assigned: jst)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

11 years ago
Created attachment 250924 [details] [diff] [review]
Better GC scheduling.
Attachment #250924 - Flags: superreview?(bugmail)
Attachment #250924 - Flags: review?(bugmail)
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.
(Assignee)

Comment 3

11 years ago
Created attachment 251129 [details] [diff] [review]
Update to Jonas' comments.

This fixes all the above issues.
Attachment #251129 - Flags: superreview?(bugmail)
Attachment #251129 - Flags: review?(bugmail)
Comment on attachment 251129 [details] [diff] [review]
Update to Jonas' comments.

Looks good, r/sr=sicking
Attachment #251129 - Flags: superreview?(bugmail)
Attachment #251129 - Flags: superreview+
Attachment #251129 - Flags: review?(bugmail)
Attachment #251129 - Flags: review+
Attachment #250924 - Attachment is obsolete: true
Attachment #250924 - Flags: superreview?(bugmail)
Attachment #250924 - Flags: review?(bugmail)
(Assignee)

Comment 5

11 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 6

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

Updated

11 years ago
Depends on: 367006

Updated

10 years ago
Depends on: 373462

Updated

10 years ago
Depends on: 373540
You need to log in before you can comment on or make changes to this bug.