Closed Bug 395045 Opened 12 years ago Closed 12 years ago

Investigate timing out of old bf-cached documents

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: roc, Assigned: roc)

Details

Attachments

(1 file, 2 obsolete files)

As part of a move towards timeout-based caching generally, for 1.9 we should try to expire bf-cached documents after they've gone unused for a certain length of time. This will help prune browser memory usage after it's been idle for a while.
Attached patch patch (obsolete) — Splinter Review
This seems to work. I wonder what is the best way to measure its effect.
Assignee: nobody → roc
Status: NEW → ASSIGNED
BTW the patch's behaviour is to expire cached contentviewers 2-3 minutes after they were last stuffed in the cache.
Attached patch updated patch (obsolete) — Splinter Review
That patch had a bad problem with destroying the expiration-tracker while we were iterating through it. This patch is simpler and safer.
Attachment #279889 - Attachment is obsolete: true
Attachment #280115 - Attachment mime type: application/text → text/plain
Attachment #280115 - Attachment is patch: true
> expire cached contentviewers 2-3 minutes after 

That seems too short.  The idea of bfcache is to give fast "back" performance.  So we'd want to keep things in the cache long enough that it would cover most cases of going back.  I doubt many users go back within 2-3 minutes of loading a document and starting to read it... but maybe that's just me.  I tend to read the new doc before going back... ;)
I don't know what the best duration would be. I wonder if there's a way to measure the distribution of back-delays among real users.
(I think there are common cases where the delay is small, like when you're searching for something and checking to see whether a result page is relevant, or when you're checking a page to see if it has been updated)
What roc said; I think those are the cases where bf is most noticeable and useful.  If you spend 10 minutes reading a page and then go back, you /probably/ won't notice the extra quarter or half second for the page to load from cache, no?
The difference between bfcache and load from cache for a large page (lots of bonsai/lxr URIs, for example) is a lot bigger than "half a second"...
Attached patch updated patchSplinter Review
I've been running with this for ages with no new problems.

This bumps the timeout to 30 minutes. We can discuss if it should be shorter after this lands.
Attachment #280115 - Attachment is obsolete: true
Attachment #281375 - Flags: superreview?(bzbarsky)
Attachment #281375 - Flags: review?(bzbarsky)
Attachment #281375 - Flags: approval1.9?
Comment on attachment 281375 [details] [diff] [review]
updated patch

>Index: docshell/shistory/src/nsSHEntry.cpp
>+#define CONTENT_VIEWER_TIMEOUT_SECONDS 30*60

Probably not worth making this pref-able, right?

>+  // Expire cached contentviewers after 2-3 minutes in the cache.

s/2-3/20-30/

>@@ -184,24 +225,26 @@ nsSHEntry::SetContentViewer(nsIContentVi
>   mContentViewer = aViewer;
> 
>   if (mContentViewer) {

And if not, we should stop tracking |this|, right?  That would mean we got restored, e.g.  In particular, I think we need to do that to avoid double adds if we get restored and then bfcached again.

>+nsSHEntry::Expire()
>+  mContentViewer->GetContainer(getter_AddRefs(container));
>+  if (!container)
>+    return;
>+  nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(container);

do_QI is null-safe.  no need to check |container|.

The rest looks fine.  It won't really work if we ever enable bfcache of subframes, but neither will most of the rest of the eviction setup we have.....

r+sr+a=bzbarsky with the above changes, but watch out for regressions. This code is famuously fragile.  :(
Attachment #281375 - Flags: superreview?(bzbarsky)
Attachment #281375 - Flags: superreview+
Attachment #281375 - Flags: review?(bzbarsky)
Attachment #281375 - Flags: review+
Attachment #281375 - Flags: approval1.9?
Attachment #281375 - Flags: approval1.9+
> And if not, we should stop tracking |this|, right?

If !aViewer then we will have called DropPresentationState which calls StopTrackingEntry.
Ah, indeed.   Sounds good then.
checked in.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Perhaps we should file a new bug to figure out the best timeout length and cc some UX people?
You need to log in before you can comment on or make changes to this bug.