395045 - Investigate timing out of old bf-cached documents

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: patch

This seems to work. I wonder what is the best way to measure its effect.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

BTW the patch's behaviour is to expire cached contentviewers 2-3 minutes after they were last stuffed in the cache.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

That patch had a bad problem with destroying the expiration-tracker while we were iterating through it. This patch is simpler and safer.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> 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... ;)

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(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)

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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"...

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated patch

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.

Comment 10

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

> And if not, we should stop tracking |this|, right?

If !aViewer then we will have called DropPresentationState which calls StopTrackingEntry.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Ah, indeed.   Sounds good then.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in.

Comment 14

[Adam Kowalczyk]

Perhaps we should file a new bug to figure out the best timeout length and cc some UX people?