775540 - [Page Thumbnails] Don't expire thumbnails less than a day old

Status: RESOLVED WONTFIX

(Firefox :: Tabbed Browser, defect)

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

I removed the EXPIRATION_MIN_CHUNK_SIZE const because I thought it didn't make sense to me anymore. We can always just remove up to a half of all thumbnails we won't keep.

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 653896 [details] [diff] [review]
patch v1

Review of attachment 653896 [details] [diff] [review]:
-----------------------------------------------------------------

I kinda liked the minimum chunk size, I think you should keep it. This way the folder can reach a full clean-up quicker, whereas dividing by half will take many more steps when approaching 0.

The changes look fine to me, but could you ask Yoric for review on the PageThumbsWorker part?  I don't know if calling `stat` will have a big overhead there, and also IIRC timestamps meaning may vary between OSes, and he should know much more about it than myself.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +17,2 @@
>  const EXPIRATION_INTERVAL_SECS = 3600;
> +const EXPIRATION_RETENTION_SECS = 86400;

add a comment saying this represents one day

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to :Felipe Gomes from comment #2)
> I kinda liked the minimum chunk size, I think you should keep it. This way
> the folder can reach a full clean-up quicker, whereas dividing by half will
> take many more steps when approaching 0.

Right... I reverted that change.

> ::: browser/components/thumbnails/PageThumbs.jsm
> @@ +17,2 @@
> >  const EXPIRATION_INTERVAL_SECS = 3600;
> > +const EXPIRATION_RETENTION_SECS = 86400;
> 
> add a comment saying this represents one day

Done.

> The changes look fine to me, but could you ask Yoric for review on the
> PageThumbsWorker part?  I don't know if calling `stat` will have a big
> overhead there, and also IIRC timestamps meaning may vary between OSes, and
> he should know much more about it than myself.

I'm sure stat calls have an overhead but that's why we do it in the worker ;) I don't know much about timestamp differences but I assumed they're corrected as .lastModificationDate returns a Date object.

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 654429 [details] [diff] [review]
patch v2

Review of attachment 654429 [details] [diff] [review]:
-----------------------------------------------------------------

I am not completely sure I understand the requirements here, so I cannot perform a review yet.

As far as I understand, this implementation has the following behavior:
- controller requests the list of files from worker ( ~n I/O for the worker thread, O(n) transfer for both threads );
- controller removes files it does not want to remove ( O(n) CPU on the main thread );
- controller sends back the list of files it wants to remove ( O(n) transfer for both threads );
- worker calls |stat| on ~half of the files of the list ( ~n/2 I/O for the worker thread );
- worker calls |remove| on some/all of these files ( ~n/2 I/O for the worker thread ).

It will work and the relevant I/O will be fully backgrounded, which is good for performance, so f+.

Now, we want to limit I/O even on background threads, because it is expensive on the battery, the hardware and the OS, and limit the communications, because they are relatively expensive (this will get better once bug 720083 has landed, but that's not done yet).

I believe that we can do better we the following approach:
- controller requests cleanup from worker, sending the list of urls to keep ( O(urls.length) for both threads );
- worker walks directory, discarding urls to keep, recording last modification if available for free ( ~n I/O for the worker thread );
- for each file in the list until we have removed ~half of the list:
  - if necessary, compute last modification date ( ~n/2 I/O for the worker thread, free under Windows );
  - if necessary, remove file ( ~n/2 I/O for the worker thread ).

That's ~25% less I/O under Windows (and one of these days also on Mac) and in this context, it will eventually be possible to optimize |stat| and |remove| under Linux/Android/B2G.

And even better:
- controller requests cleanup from worker, sending the list of urls to keep ( O(urls.length) for both threads );
- if this is the first time, worker walks directory, building a map of files currently in the directory, recording their last modification if available for free ( ~n I/O for the worker thread, only the first time );
- for each file in the map until we have removed ~half of the map:
  - if necessary, compute (and store) last modification date ( ~n/2 I/O for the worker thread, free under Windows );
  - if necessary, remove file from disk and from map (~n/2 I/O for the worker thread).

That's ~50% less I/O on all platforms in permanent mode.

Now, this approach requires the map to be updated each time we add/remove a file, which requires porting PageThumbsStorage.write and PageThumbsStorage.copy to use OS.File. It is probably a good idea anyway, but I assume that you would prefer doing this in a followup bug.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +454,1 @@
>        keep[PageThumbsStorage.getLeafNameForURL(url)] = true;

I would rather use a |Map| or a |Dict|. This way, if we ever change |getLeafNameForURL|, we will not risk a weird security error if it returns, say, "__proto__".

@@ +454,5 @@
>        keep[PageThumbsStorage.getLeafNameForURL(url)] = true;
>      }
>  
>      let numFilesRemoved = 0;
>      let dir = PageThumbsStorage.getDirectory().path;

Once we have bug 781346, we will be able to compute |dir| completely on the worker. For the moment, though, I believe that we can simply send |dir| during initialization of the worker.

@@ +463,3 @@
>  
> +      let msg = {
> +        paths: paths,

If I understand correctly, we are sending back all paths even if we only need half of them. This could be improved.

::: browser/components/thumbnails/PageThumbsWorker.js
@@ +67,2 @@
>        try {
> +        let info = OS.File.stat(file);

If we collapse the call to |getFilesInDirectory| and |removeFiles|, we could optimize this as follows:

- for Windows platforms, by using field |winLastModificationDate| to completely avoid the call to |stat|;
- in the future, for Mac platforms, there will be a field |macLastModificationDate| with the same properties;
- in the future, for Linux/Android/B2G, there will be a method that can perform a faster variant of |stat| while walking a directory (name not chosen yet, but this is basically |fstatat()|).

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

(In reply to David Rajchenbach Teller [:Yoric] from comment #4)
> I believe that we can do better we the following approach:
> - controller requests cleanup from worker, sending the list of urls to keep
> ( O(urls.length) for both threads );
> - worker walks directory, discarding urls to keep, recording last
> modification if available for free ( ~n I/O for the worker thread );
> - for each file in the list until we have removed ~half of the list:
>   - if necessary, compute last modification date ( ~n/2 I/O for the worker
> thread, free under Windows );
>   - if necessary, remove file ( ~n/2 I/O for the worker thread ).

I did exactly this. I like your second approach as well but we should really do it when all the storage uses the IO worker.

> ::: browser/components/thumbnails/PageThumbs.jsm
> @@ +454,1 @@
> >        keep[PageThumbsStorage.getLeafNameForURL(url)] = true;
> 
> I would rather use a |Map| or a |Dict|. This way, if we ever change
> |getLeafNameForURL|, we will not risk a weird security error if it returns,
> say, "__proto__".

Good point, "Set" is a lot cooler to use, thanks.

> If we collapse the call to |getFilesInDirectory| and |removeFiles|, we could
> optimize this as follows:
> 
> - for Windows platforms, by using field |winLastModificationDate| to
> completely avoid the call to |stat|;
> - in the future, for Mac platforms, there will be a field
> |macLastModificationDate| with the same properties;
> - in the future, for Linux/Android/B2G, there will be a method that can
> perform a faster variant of |stat| while walking a directory (name not
> chosen yet, but this is basically |fstatat()|).

Did this, too. I didn't include a check for "macLastModificationDate" unless you tell me it's gonna be named like that for sure :)

The field for windows is actually called "winLastWriteDate".

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 654843 [details] [diff] [review]
patch v3

Review of attachment 654843 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me!
By the way, is there a testsuite (or better even, a benchmark) for this feature?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #6)
> By the way, is there a testsuite (or better even, a benchmark) for this
> feature?

No.. I was planning on writing tests for the whole expiration mechanism in a follow-up bug. Is that okay for you?

Comment 8

[Dão Gottwald [::dao]]

In what cases are the current heuristics insufficient, i.e. why do we need this bug?

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Dão Gottwald [:dao] from comment #8)
> In what cases are the current heuristics insufficient, i.e. why do we need
> this bug?

After thinking about this a little, we shouldn't do this. Actually this was planned to preserve thumbnails for features that may need them after the thumbnails normally would have been expired. But we have a flexible expiration mechanism. If a feature needs to preserve thumbnails for some reason it should implement its own ExpirationFilter.

What I'd like to take from the patch in this bug is the performance improvements regarding the chrome <-> worker communication. Will change the patch accordingly.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

> What I'd like to take from the patch in this bug is the performance
> improvements regarding the chrome <-> worker communication. Will change the
> patch accordingly.

Filed bug 794804.