Closed Bug 775540 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #653896 - Flags: review?(felipc)
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
Attached patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #653896 - Attachment is obsolete: true
Attachment #653896 - Flags: review?(felipc)
Attachment #654429 - Flags: review?(dteller)
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()|).
Attachment #654429 - Flags: review?(dteller) → feedback+
Attached patch patch v3Splinter Review
(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".
Attachment #654429 - Attachment is obsolete: true
Attachment #654843 - Flags: review?(dteller)
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?
Attachment #654843 - Flags: review?(dteller) → review+
(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?
In what cases are the current heuristics insufficient, i.e. why do we need this bug?
(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.
> 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.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.