753768 - [Page thumbnails] Move all I/O off the main thread

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, enhancement)

Description

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

At the moment Page Thumbnails custom storage uses NetUtils + xpcom.

For better performance, we should port it to off-main thread OS.File.

Comment 1

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

Attachment: First prototype

Attaching a first draft.

Comment 2

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

Attachment: Implementing asynchronous |write|, |copy|, |remove|.

Here is a first version that seems to work nicely. I have not implemented asynchronous rmdir yet, as OS.File does not offer portable, fast, rmdir yet.

Comment 3

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

Attachment: Implementing asynchronous |write|, |copy|, |remove|.

All tests seem to pass, except browser_thumbnails_bug753755 – which I suspect did not pass either before my patch.

Comment 4

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

Comment on attachment 630483 [details] [diff] [review]
Implementing asynchronous |write|, |copy|, |remove|.

Ironing out a few issues.

Comment 5

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

Attachment: Implementing asynchronous |write|, |copy|, |remove|.

Comment 6

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

Attachment: Companion testsuite

Comment 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 630990 [details] [diff] [review]
Implementing asynchronous |write|, |copy|, |remove|.

>diff --git a/browser/components/thumbnails/Makefile.in b/browser/components/thumbnails/Makefile.in

>+libs::
>+	$(NSINSTALL) $(srcdir)/PageThumbsWorker.js $(FINAL_TARGET)/modules/

This should use EXTRA_JS_MODULES.

>diff --git a/browser/components/thumbnails/PageThumbs.jsm b/browser/components/thumbnails/PageThumbs.jsm

>-  get scheme() "moz-page-thumb",
>+  get scheme() {
>+    return "moz-page-thumb";
>+  },

This change (and others like it) doesn't seem useful.

>diff --git a/browser/components/thumbnails/PageThumbsWorker.js b/browser/components/thumbnails/PageThumbsWorker.js

>+/**
>+ * A worker dedicated for the I/O component of PageThumbs storage.
>+ *
>+ * Do not rely on the API of this worker. In a future version, it might be
>+ * fully replaced by a OS.File global I/O worker.
>+ */

What are OS.File global I/O workers?

I really don't like how thumbnail service code is needing to implement its own worker to do async I/O via this API. Why can't we have a single, centralized module that takes care of this worker instantiation/message passing and exposes a much more reasonable API (e.g. AsyncIO.copyFile, AsyncIO.removeFile, etc.) to external consumers? Is that what you're referring to by "OS.File global I/O workers"?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 630990 [details] [diff] [review]
Implementing asynchronous |write|, |copy|, |remove|.

Cancelling review as per conversation on IRC. We'll want to wait for bug 754671 to land first so that we can uplift it to Aurora if we don't make it in time for Fx 16. The other patch will already introduce the chrome worker so we'd need to update the patches here afterwards.

Comment 9

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

Attachment: Implementing asynchronous |write|, |copy|.

Comment 10

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

Attachment: Moving all I/O off the main thread

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 685191 [details] [diff] [review]
Moving all I/O off the main thread

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

Didn't look at the JSM yet but wanted to drop my thoughts about the worker and the test.

::: browser/components/thumbnails/PageThumbsWorker.js
@@ +18,3 @@
>  
> +/**
> + * Communications with the controller.

Nit: Communicates with the controller? Communication with the controller?

@@ +24,5 @@
> + *
> + * Sends messages:
> + * {ok: result} / {fail: serialized_form_of_OS.File.Error}
> + */
> +self.onmessage = function onmessage(msg) {

I wondered if this whole function could maybe be re-used for other code that communicates with the controller? This is probably always the same, right? We could do something like:

|self.onmessage = MessageHandler(Agent)|

@@ +40,1 @@
>      }

Instead of having |if (DEBUG) LOG()| everywhere, couldn't we just define a debug() function at the top that calls LOG()? It can either check DEBUG or we just comment out the LOG call for production.

@@ +40,2 @@
>      }
> +    result = Agent[method].apply(Agent, data.args);

We could maybe get the method and check if (method in Agent) outside of the try block? That makes it a little clearer which code we expect to fail.

@@ +56,5 @@
> +    if (DEBUG) {
> +      LOG("Sending positive reply", JSON.stringify(result), "id is", id);
> +    }
> +    self.postMessage({ok: result, id:id});
> +  } else if (exn == StopIteration) {

We can handle this all a little nicer like this:

try {
  // try stuff
} catch (e if e instanceof StopIteration) {
  // handle StopIteration
} catch (e if e instanceof exports.OS.File.Error) {
  // handle OS.File.Error
}

That's a nicer structure and other exception types aren't catched so we don't need to explicitly throw them again.

@@ +84,3 @@
>  
> +let Agent = {
> +  remove: function Worker_removeFile(path) {

Agent_removeFile.

@@ +88,4 @@
>    },
>  
> +  expireFilesInDirectory:
> +    function Worker_expireFilesInDirectory(minChunkSize, path, filesToKeep) {

Agent_expireFilesInDirectory

@@ +104,5 @@
>      return true;
>    },
>  
>    getFileEntriesInDirectory:
> +  function Worker_getFileEntriesInDirectory(path, skipFiles) {

Agent_getFileEntriesInDirectory

@@ +114,5 @@
>              if (!entry.isDir && !entry.isSymLink && !skip.has(entry.name))];
> +  },
> +
> +  writeAtomic:
> +  function Worker_writeAtomic(path, buffer, options) {

Agent_writeAtomic

@@ +121,5 @@
> +      options);
> +  },
> +
> +  makeDir:
> +  function Worker_makeDir(path, options) {

Agent_makeDir

@@ +126,5 @@
> +    return OS.File.makeDir(path, options);
> +  },
> +
> +  copy:
> +  function Worker_copy(source, dest) {

Agent_copy

@@ +131,5 @@
> +    return OS.File.copy(source, dest);
> +  },
> +
> +  wipe:
> +  function Worker_wipe(path) {

Agent_wipe

::: browser/components/thumbnails/test/browser_thumbnails_expiration.js
@@ +46,5 @@
>    let urls = [];
>    for (let i = 0; i < EXPIRATION_MIN_CHUNK_SIZE + 10; i++) {
> +    let url = URL + "#dummy" + i;
> +    urls.push(url);
> +    yield createDummyThumbnail(urls);

This should be |createDummyThumbnail(url)|, no? If the test is not failing here it's probably doing something wrong.

Comment 12

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

(In reply to Tim Taubert [:ttaubert] from comment #11)
> Comment on attachment 685191 [details] [diff] [review]
> Moving all I/O off the main thread
> 
> Review of attachment 685191 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Didn't look at the JSM yet but wanted to drop my thoughts about the worker
> and the test.
> 
> ::: browser/components/thumbnails/PageThumbsWorker.js
> @@ +18,3 @@
> >  
> > +/**
> > + * Communications with the controller.
> 
> Nit: Communicates with the controller? Communication with the controller?
> 
> @@ +24,5 @@
> > + *
> > + * Sends messages:
> > + * {ok: result} / {fail: serialized_form_of_OS.File.Error}
> > + */
> > +self.onmessage = function onmessage(msg) {
> 
> I wondered if this whole function could maybe be re-used for other code that
> communicates with the controller? This is probably always the same, right?
> We could do something like:
> 
> |self.onmessage = MessageHandler(Agent)|

I have opened bug 801598 on that topic. This should be feasible.

> @@ +40,1 @@
> >      }
> 
> Instead of having |if (DEBUG) LOG()| everywhere, couldn't we just define a
> debug() function at the top that calls LOG()? It can either check DEBUG or
> we just comment out the LOG call for production.

Some calls to LOG require a JSON.stringify, which is rather expensive, so I preferred prefixing everything with |if (DEBUG)|. Of course, I could add a function |stringify| that behaves as JSON.stringify if DEBUG is set or noop  if it isn't. Would you prefer that?

> 
> @@ +40,2 @@
> >      }
> > +    result = Agent[method].apply(Agent, data.args);
> 
> We could maybe get the method and check if (method in Agent) outside of the
> try block? That makes it a little clearer which code we expect to fail.

Ok.

> 
> @@ +56,5 @@
> > +    if (DEBUG) {
> > +      LOG("Sending positive reply", JSON.stringify(result), "id is", id);
> > +    }
> > +    self.postMessage({ok: result, id:id});
> > +  } else if (exn == StopIteration) {
> 
> We can handle this all a little nicer like this:
> 
> try {
>   // try stuff
> } catch (e if e instanceof StopIteration) {
>   // handle StopIteration
> } catch (e if e instanceof exports.OS.File.Error) {
>   // handle OS.File.Error
> }
> 
> That's a nicer structure and other exception types aren't catched so we
> don't need to explicitly throw them again.

That looks good, thanks. Do you know if this is standard JS?

[...]

> ::: browser/components/thumbnails/test/browser_thumbnails_expiration.js
> @@ +46,5 @@
> >    let urls = [];
> >    for (let i = 0; i < EXPIRATION_MIN_CHUNK_SIZE + 10; i++) {
> > +    let url = URL + "#dummy" + i;
> > +    urls.push(url);
> > +    yield createDummyThumbnail(urls);
> 
> This should be |createDummyThumbnail(url)|, no? If the test is not failing
> here it's probably doing something wrong.

Good point. I will take a look.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> Some calls to LOG require a JSON.stringify, which is rather expensive, so I
> preferred prefixing everything with |if (DEBUG)|. Of course, I could add a
> function |stringify| that behaves as JSON.stringify if DEBUG is set or noop 
> if it isn't. Would you prefer that?

Oh, I see. Hm. We could have a no-op stringify function or check all passed arguments and stringify all objects. Feel free to choose what you like the most.

> That looks good, thanks. Do you know if this is standard JS?

Good question. From https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Statements/try...catch I think this was introduced with JS 1.5. Should *probably* be working in other browsers as well?

Comment 14

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

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> > This should be |createDummyThumbnail(url)|, no? If the test is not failing
> > here it's probably doing something wrong.
> 
> Good point. I will take a look.

I double-checked, and there is nothing to worry about. Just a last minute code cleanup typo that was effectively caught by the test suite.

> I wondered if this whole function could maybe be re-used for other code that
> communicates with the controller? This is probably always the same, right?
> We could do something like:
> 
> |self.onmessage = MessageHandler(Agent)|

Do you mind if we handle this as a followup bug?

Comment 15

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

Attachment: Moving all I/O off the main thread, v2

Comment 16

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

Attachment: Moving all I/O off the main thread, v3

I have applied your feedback and taken the opportunity to fix deserialization of exceptions.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #14)
> > I wondered if this whole function could maybe be re-used for other code that
> > communicates with the controller? This is probably always the same, right?
> > We could do something like:
> > 
> > |self.onmessage = MessageHandler(Agent)|
> 
> Do you mind if we handle this as a followup bug?

Of course not. I was just thinking out loud.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #16)
> Created attachment 687379 [details] [diff] [review]
> Moving all I/O off the main thread

The v2 patch is now deprecated, right?

Comment 19

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

Comment on attachment 687379 [details] [diff] [review]
Moving all I/O off the main thread, v3

Oops.

Comment 20

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

Review ping?

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 687379 [details] [diff] [review]
Moving all I/O off the main thread, v3

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

Thanks for your hard work on this. Looks really promising (pun intended :). There's a couple of questions and things that need fixing but nothing too serious.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +78,5 @@
> +        Cu.reportError("Uncaught asynchronous error: " + reason + " at\n" + reason.stack + "\n");
> +        throw reason;
> +      }
> +    );
> +  },

Nit: newline, please.

@@ +92,5 @@
> +   * as |Task.spawn(gen)|.
> +   */
> +  spawn: function spawn(gen) {
> +    return this.captureErrors(Task.spawn(gen));
> +  },

Nit: newline, please.

@@ +264,5 @@
>      let url = aBrowser.currentURI.spec;
>      let channel = aBrowser.docShell.currentDocumentChannel;
>      let originalURL = channel.originalURI.spec;
>  
> +    let self = this;

Please use .bind(this).

@@ +266,5 @@
>      let originalURL = channel.originalURI.spec;
>  
> +    let self = this;
> +    TaskUtils.spawn(function task() {
> +      let exn;

We're not really using the value of 'exn'. Why not have a variable like 'success' that is set to 'false' when we catch an exception?

@@ +389,5 @@
> +  getPath: function Storage_getPath(aCreate = true) {
> +    // FIXME: This should be localProfileDir, pending bug 781346
> +    if (!this._path && aCreate) {
> +      this._path =
> +        OS.Path.join(OS.Constants.Path.profileDir, THUMBNAIL_DIRECTORY);

Wait, if (!this._path) we should always at least store the file path, right? Independently, we should then create the directory when aCreate=true. Creating the directory is asynchronous, should we maybe split this into to methods and the creating one returns a promise?

@@ +410,5 @@
>    getLeafNameForURL: function Storage_getLeafNameForURL(aURL) {
> +    if (typeof aURL != "string") {
> +      dump("ERROR: " + new Error().stack + "\n");
> +      throw new TypeError("Expecting a string");
> +    }

This looks more like debug code?

@@ +415,5 @@
>      let hash = this._calculateMD5Hash(aURL);
>      return hash + ".png";
>    },
>  
> +  getPathForURL: function Storage_getPathForURL(aURL) {

Maybe |getFilePathForURL|? Makes it a little clearer that we're getting the path of a single file here.

@@ +417,5 @@
>    },
>  
> +  getPathForURL: function Storage_getPathForURL(aURL) {
> +    let result = OS.Path.join(this.getPath(), this.getLeafNameForURL(aURL));
> +    return result;

Nit: let's just return this without a variable we don't really need.

@@ +625,5 @@
> +
> +    return PageThumbsWorker.post(
> +      "expireFilesInDirectory",
> +      msg,
> +      msg

I don't understand why we're passing the msg argument twice just like all the other Worker.post() calls. Do we really need that?

@@ +631,4 @@
>    }
>  };
>  
> +Components.utils.import("resource://gre/modules/osfile.jsm", this);

Nit: Cu.import() and we should better move this to the top where the other imports are.

::: browser/components/thumbnails/PageThumbsWorker.js
@@ +14,5 @@
>  importScripts("resource://gre/modules/osfile.jsm");
>  
> +let DEBUG = false;
> +let LOG = OS.Shared.LOG.bind(null, "PageThumbsWorker");
> +let stringify = function stringify(obj) {

Nit: We don't really need the |let stringify = | part to define a function.

@@ +35,5 @@
> +  let data = msg.data;
> +  LOG("Received message", JSON.stringify(data));
> +  let id = data.id;
> +  let result;
> +  let exn;

That variable is now unused.

@@ +37,5 @@
> +  let id = data.id;
> +  let result;
> +  let exn;
> +  let fun = Agent[data.fun];
> +  if (!fun) {

Nit: If we'd check with |if (data.fun in Agent)| we wouldn't provoke a warning in the rare case the function doesn't exist.

@@ +46,5 @@
> +    result = fun.apply(Agent, data.args);
> +  } catch (ex if ex instanceof StopIteration) {
> +    // StopIteration cannot be serialized automatically
> +    LOG("Sending back StopIteration");
> +    self.postMessage({StopIteration: true, id: id});

There's a return missing here or we would send two messages.

@@ +48,5 @@
> +    // StopIteration cannot be serialized automatically
> +    LOG("Sending back StopIteration");
> +    self.postMessage({StopIteration: true, id: id});
> +  } catch (ex if ex instanceof exports.OS.File.Error) {
> +    LOG("Sending back OS.File error", exn, "id is", id);

Use 'ex' instead of 'exn'.

@@ +52,5 @@
> +    LOG("Sending back OS.File error", exn, "id is", id);
> +    // Instances of OS.File.Error know how to serialize themselves
> +    // (deserialization ensures that we end up with OS-specific
> +    // instances of |OS.File.Error|)
> +    self.postMessage({fail: exports.OS.File.Error.toMsg(ex), id:id});

There's a return missing here or we would send two messages.

@@ +97,5 @@
>              if (!entry.isDir && !entry.isSymLink && !skip.has(entry.name))];
> +  },
> +
> +  writeAtomic:
> +  function Agent_writeAtomic(path, buffer, options) {

Nit: We should only wrap lines if the line gets too long otherwise. Same for the functions below.

Comment 22

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

(In reply to Tim Taubert [:ttaubert] from comment #21)
> @@ +92,5 @@
> > +   * as |Task.spawn(gen)|.
> > +   */
> > +  spawn: function spawn(gen) {
> > +    return this.captureErrors(Task.spawn(gen));
> > +  },
> 
> Nit: newline, please.

Sure. Where?

> 
> @@ +389,5 @@
> > +  getPath: function Storage_getPath(aCreate = true) {
> > +    // FIXME: This should be localProfileDir, pending bug 781346
> > +    if (!this._path && aCreate) {
> > +      this._path =
> > +        OS.Path.join(OS.Constants.Path.profileDir, THUMBNAIL_DIRECTORY);
> 
> Wait, if (!this._path) we should always at least store the file path, right?
> Independently, we should then create the directory when aCreate=true.
> Creating the directory is asynchronous, should we maybe split this into to
> methods and the creating one returns a promise?

Yes, you are right, this is fishy. Fixing the function.

Now, for the module, we don't need to care about asynchronicity of directory creation, because all further I/O is going to be serialized by PageThumbsWorker. If you want me to store a promise somewhere, I can certainly do it, but I am not sure if it would be useful.

> @@ +625,5 @@
> > +
> > +    return PageThumbsWorker.post(
> > +      "expireFilesInDirectory",
> > +      msg,
> > +      msg
> 
> I don't understand why we're passing the msg argument twice just like all
> the other Worker.post() calls. Do we really need that?

Low-level magic, actually only useful for writeData. Cleaning this up. 

> ::: browser/components/thumbnails/PageThumbsWorker.js
> @@ +14,5 @@
> >  importScripts("resource://gre/modules/osfile.jsm");
> >  
> > +let DEBUG = false;
> > +let LOG = OS.Shared.LOG.bind(null, "PageThumbsWorker");
> > +let stringify = function stringify(obj) {
> 
> Nit: We don't really need the |let stringify = | part to define a function.

Not in this case, no. IIRC, however, if we are in strict mode *and* we define the function inside another function, this is required. Given that defining functions inside functions is the only way of getting any modularity in either web context or workers, I took the habit of doing this. Do you want me to remove it?

> @@ +97,5 @@
> >              if (!entry.isDir && !entry.isSymLink && !skip.has(entry.name))];
> > +  },
> > +
> > +  writeAtomic:
> > +  function Agent_writeAtomic(path, buffer, options) {
> 
> Nit: We should only wrap lines if the line gets too long otherwise. Same for
> the functions below.

Hey, I got that from your code :)

Comment 23

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

Attachment: Moving all I/O off the main thread, v3

Same code, with feedback applied, and now using localProfileDir.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #22)
> Not in this case, no. IIRC, however, if we are in strict mode *and* we
> define the function inside another function, this is required. Given that
> defining functions inside functions is the only way of getting any
> modularity in either web context or workers, I took the habit of doing this.
> Do you want me to remove it?

Even in strict mode we still can define functions at the top-level of functions. This works:

function foo() {
  function bar() {
  }

  bar();
}

This doesn't:

function foo() {
  if (true) {
    function bar() {
    }

    bar();
  }
}

> > > +  writeAtomic:
> > > +  function Agent_writeAtomic(path, buffer, options) {
> > 
> > Nit: We should only wrap lines if the line gets too long otherwise. Same for
> > the functions below.
> 
> Hey, I got that from your code :)

Heh, I don't mind "fixing" my own code :)

Comment 25

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

(In reply to Tim Taubert [:ttaubert] from comment #24)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #22)
> > Not in this case, no. IIRC, however, if we are in strict mode *and* we
> > define the function inside another function, this is required. Given that
> > defining functions inside functions is the only way of getting any
> > modularity in either web context or workers, I took the habit of doing this.
> > Do you want me to remove it?
> 
> Even in strict mode we still can define functions at the top-level of
> functions. This works:
> 
> function foo() {
>   function bar() {
>   }
> 
>   bar();
> }
> 
> This doesn't:
> 
> function foo() {
>   if (true) {
>     function bar() {
>     }
> 
>     bar();
>   }
> }

Ah, missed that one. Well, I can remove the |let| part if you prefer.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #22)
> (In reply to Tim Taubert [:ttaubert] from comment #21)
> > > +   * as |Task.spawn(gen)|.
> > > +   */
> > > +  spawn: function spawn(gen) {
> > > +    return this.captureErrors(Task.spawn(gen));
> > > +  },
> > 
> > Nit: newline, please.
> 
> Sure. Where?

Sorry, I meant between the function's doc comments and the previous function definition.

> > Wait, if (!this._path) we should always at least store the file path, right?
> > Independently, we should then create the directory when aCreate=true.
> > Creating the directory is asynchronous, should we maybe split this into to
> > methods and the creating one returns a promise?
> 
> Now, for the module, we don't need to care about asynchronicity of directory
> creation, because all further I/O is going to be serialized by
> PageThumbsWorker.

Hm that's true but can yield interesting results if we have a foreign caller that just assumes the path has been created synchronously. Should we maybe have a separate function like "makePath" or "createPath" that returns a promise and is called by the callers that care about that the path actually exists?

(In reply to David Rajchenbach Teller [:Yoric] from comment #25)
> Ah, missed that one. Well, I can remove the |let| part if you prefer.

Why not :P

Comment 27

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

Attachment: Moving all I/O off the main thread, v4

Here we go.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 693861 [details] [diff] [review]
Moving all I/O off the main thread, v4

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

In the test browser_thumbnails_storage.js there are two occurences of:

> PageThumbsStorage.copy(URL, URL_COPY);

which we need to turn into something like:

> yield PageThumbsStorage.copy(URL, URL_COPY).then(next);

Tests run successfully on my machine with those changes.

::: browser/components/thumbnails/PageThumbs.jsm
@@ +410,5 @@
> +   */
> +  getPath: function Storage_getPath(aCreate = true) {
> +    if (aCreate) {
> +      this.mkPath();
> +    }

I think |getPath()| should not have an |aCreate| parameter. Callers should make sure the path exists themselves by explicitly calling |mkPath()| before |getPath()|. Maybe we don't need this function at all and we just access |PageThumbsStorage.path|?

Actually, writeData, copy, remove and wipe would need to ensure that the thumbnails directory exists before doing anything. Well, writeData and copy, remove and wipe don't really need a directory.

This sounds like something the worker could do when it is initialized... Maybe the first message we send is "make sure the directory exists". OTOH this of course doesn't cover the case when I remove the directory while Firefox is already running.

@@ +484,5 @@
> +    return PageThumbsWorker.post("wipe", [this.path]);
> +  },
> +
> +  // Deprecated
> +  getDirectory: function Storage_getDirectory(aCreate = true) {

We can just remove those functions, right? I don't think we have any callers left.

@@ +489,5 @@
> +    return new FileUtils.File(this.getPath(aCreate));
> +  },
> +
> +  // Deprecated
> +  getFileForURL: function Storage_getFilePathForURL(aURL) {

^

@@ +499,1 @@
>    write: function Storage_write(aURL, aDataStream, aCallback) {

^

::: browser/components/thumbnails/PageThumbsWorker.js
@@ +33,5 @@
> + * {ok: result} / {fail: serialized_form_of_OS.File.Error}
> + */
> +self.onmessage = function onmessage(msg) {
> +  let data = msg.data;
> +  LOG("Received message", JSON.stringify(data));

We should use just |stringify()| here, right?

@@ +42,5 @@
> +    throw new Error("Cannot find method " + data.fun);
> +  }
> +  let fun = Agent[data.fun];
> +  try {
> +    result = fun.apply(Agent, data.args);

Sorry for being picky but we could just do:

> Agent[data.fun].apply(Agent, data.args);

We could also move |let fun = ...| into the try-catch clause. It's just a little more explicit that we don't use this variable elsewhere (I was looking where we use that again).

Comment 29

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

(In reply to Tim Taubert [:ttaubert] from comment #28)
> Comment on attachment 693861 [details] [diff] [review]
> Moving all I/O off the main thread, v4
> 
> Review of attachment 693861 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In the test browser_thumbnails_storage.js there are two occurences of:
> 
> > PageThumbsStorage.copy(URL, URL_COPY);
> 
> which we need to turn into something like:
> 
> > yield PageThumbsStorage.copy(URL, URL_COPY).then(next);
> 
> Tests run successfully on my machine with those changes.

Done.

> ::: browser/components/thumbnails/PageThumbs.jsm
> @@ +410,5 @@
> > +   */
> > +  getPath: function Storage_getPath(aCreate = true) {
> > +    if (aCreate) {
> > +      this.mkPath();
> > +    }
> 
> I think |getPath()| should not have an |aCreate| parameter. Callers should
> make sure the path exists themselves by explicitly calling |mkPath()| before
> |getPath()|. Maybe we don't need this function at all and we just access
> |PageThumbsStorage.path|?
> 
> Actually, writeData, copy, remove and wipe would need to ensure that the
> thumbnails directory exists before doing anything. Well, writeData and copy,
> remove and wipe don't really need a directory.
> 
> This sounds like something the worker could do when it is initialized...
> Maybe the first message we send is "make sure the directory exists". OTOH
> this of course doesn't cover the case when I remove the directory while
> Firefox is already running.

I have kept it along with writeData and copy, to ensure that we don't do weird stuff if the directory is removed.

> 
> @@ +484,5 @@
> > +    return PageThumbsWorker.post("wipe", [this.path]);
> > +  },
> > +
> > +  // Deprecated
> > +  getDirectory: function Storage_getDirectory(aCreate = true) {
> 
> We can just remove those functions, right? I don't think we have any callers
> left.
> 
> @@ +489,5 @@
> > +    return new FileUtils.File(this.getPath(aCreate));
> > +  },
> > +
> > +  // Deprecated
> > +  getFileForURL: function Storage_getFilePathForURL(aURL) {

Yeah, well, there were a dozen or so callers, but I removed them.

> ^
> 
> @@ +499,1 @@
> >    write: function Storage_write(aURL, aDataStream, aCallback) {
> 
> ^
> 
> ::: browser/components/thumbnails/PageThumbsWorker.js
> @@ +33,5 @@
> > + * {ok: result} / {fail: serialized_form_of_OS.File.Error}
> > + */
> > +self.onmessage = function onmessage(msg) {
> > +  let data = msg.data;
> > +  LOG("Received message", JSON.stringify(data));
> 
> We should use just |stringify()| here, right?

Removed all logging anyway.

> @@ +42,5 @@
> > +    throw new Error("Cannot find method " + data.fun);
> > +  }
> > +  let fun = Agent[data.fun];
> > +  try {
> > +    result = fun.apply(Agent, data.args);
> 
> Sorry for being picky but we could just do:
> 
> > Agent[data.fun].apply(Agent, data.args);
> 
> We could also move |let fun = ...| into the try-catch clause. It's just a
> little more explicit that we don't use this variable elsewhere (I was
> looking where we use that again).

Done.

Comment 30

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

Attachment: Moving all I/O off the main thread, v5

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 725007 [details] [diff] [review]
Moving all I/O off the main thread, v5

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

Re browser_thumbnails_storage.js, it would be great if the TestRunner could handle when we yield promises, just like Task.jsm. We could do it like:

>  next: function () {
>    try {
>      let value = TestRunner._iter.next();
>      if (value && typeof value.then == "function")
>        value.then(next);
>    } catch (e if e instanceof StopIteration) {
>      finish();
>    }
>  }

Looks great otherwise if we fix those small issues. Sorry that it took me so long!

::: toolkit/components/thumbnails/PageThumbs.jsm
@@ +413,5 @@
> +    }
> +    return this._path;
> +  },
> +
> +  mkPath: function Storage_mkPath() {

How about we name this .ensurePath()? It's indeed creating a directory but we use it to ensure there is a path before writing/copying.

@@ +566,5 @@
>     */
> +  migrateToVersion3: function Migrator_migrateToVersion3(
> +    local = OS.Constants.Path.localProfileDir,
> +    roaming = OS.Constants.Path.profileDir) {
> +    PageThumbsWorker.post(

This should really only happen if (local != roaming).

::: toolkit/components/thumbnails/PageThumbsWorker.js
@@ +62,5 @@
>      }
>    },
>  
> +  expireFilesInDirectory:
> +  function Agent_expireFilesInDirectory(minChunkSize, path, filesToKeep) {

I'd rather have "minChunkSize" as the last argument here.

@@ +100,2 @@
>        return true;
> +    }

Why do we check this in the worker? We can prevent the extra work if we do that in the JSM.

@@ +100,4 @@
>        return true;
> +    }
> +    let iter = new OS.File.DirectoryIterator(pathFrom);
> +    if (iter.exists()) {

We could actually re-use Agent.getFileEntriesInDirectory() here, no?

@@ +147,5 @@
> +    let iterator = new File.DirectoryIterator(path);
> +    try {
> +      for (let entry in iterator) {
> +        File.remove(entry.path);
> +      }

If we can't remove a single file for some reason we should still try to remove the remaining.

::: toolkit/components/thumbnails/test/browser_thumbnails_storage.js
@@ +21,5 @@
>    yield clearHistory();
>    yield createThumbnail();
>  
>    // Make sure Storage.copy() updates an existing file.
>    PageThumbsStorage.copy(URL, URL_COPY);

Needs to be yield PageThumbsStorage.copy(URL, URL_COPY);

@@ +27,3 @@
>    let mtime = copy.lastModifiedTime -= 60;
>  
>    PageThumbsStorage.copy(URL, URL_COPY);

Needs to be yield PageThumbsStorage.copy(URL, URL_COPY);

::: toolkit/components/thumbnails/test/head.js
@@ +89,5 @@
>   */
>  function whenLoaded(aElement, aCallback) {
>    aElement.addEventListener("load", function onLoad() {
>      aElement.removeEventListener("load", onLoad, true);
> +    SimpleTest.executeSoon(aCallback || next);

(pfft)

@@ +212,5 @@
>   * @param aURL The URL of the thumbnail's page.
>   * @param [optional] aCallback
>   *        Function to be invoked on completion.
>   */
> +function whenFileExists(aURL, aCallback = next, depth = 0) {

What's |depth| for?

@@ +218,5 @@
>    if (!thumbnailExists(aURL)) {
>      callback = function () whenFileExists(aURL, aCallback);
>    }
>  
>    executeSoon(callback || next);

That default value for aCallback is nice - we don't need the fallback here if we have that.

Comment 32

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

(In reply to Tim Taubert [:ttaubert] (at the Performance work week, may not respond until Mar 25th) from comment #31)
> Comment on attachment 725007 [details] [diff] [review]
> Moving all I/O off the main thread, v5
> 
> Review of attachment 725007 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Re browser_thumbnails_storage.js, it would be great if the TestRunner could
> handle when we yield promises, just like Task.jsm. We could do it like:
> 
> >  next: function () {
> >    try {
> >      let value = TestRunner._iter.next();
> >      if (value && typeof value.then == "function")
> >        value.then(next);
> >    } catch (e if e instanceof StopIteration) {
> >      finish();
> >    }
> >  }
> 
> Looks great otherwise if we fix those small issues. Sorry that it took me so
> long!
> 
> ::: toolkit/components/thumbnails/PageThumbs.jsm
> @@ +413,5 @@
> > +    }
> > +    return this._path;
> > +  },
> > +
> > +  mkPath: function Storage_mkPath() {
> 
> How about we name this .ensurePath()? It's indeed creating a directory but
> we use it to ensure there is a path before writing/copying.

Sure.

> @@ +566,5 @@
> >     */
> > +  migrateToVersion3: function Migrator_migrateToVersion3(
> > +    local = OS.Constants.Path.localProfileDir,
> > +    roaming = OS.Constants.Path.profileDir) {
> > +    PageThumbsWorker.post(
> 
> This should really only happen if (local != roaming).

As you have noticed, I have put the check in the worker.

> 
> ::: toolkit/components/thumbnails/PageThumbsWorker.js
> @@ +62,5 @@
> >      }
> >    },
> >  
> > +  expireFilesInDirectory:
> > +  function Agent_expireFilesInDirectory(minChunkSize, path, filesToKeep) {
> 
> I'd rather have "minChunkSize" as the last argument here.

Done.

> 
> @@ +100,2 @@
> >        return true;
> > +    }
> 
> Why do we check this in the worker? We can prevent the extra work if we do
> that in the JSM.

I have attempted to keep the semantics of expireFilesInDirectory without increasing the number of threads communications. If we port literally, we have:
- one message to create the directory;
- if local != roaming, one message to migrate.

Here, I have ported with one single message, that will always create the directory and, if local != roaming, migrate. I suspect that this is actually less work (ok, a few more bytes to transmit, but only one message).

> @@ +100,4 @@
> >        return true;
> > +    }
> > +    let iter = new OS.File.DirectoryIterator(pathFrom);
> > +    if (iter.exists()) {
> 
> We could actually re-use Agent.getFileEntriesInDirectory() here, no?

We probably could, but I don't think that this would make things any clearer (or faster). Your call.

> 
> @@ +147,5 @@
> > +    let iterator = new File.DirectoryIterator(path);
> > +    try {
> > +      for (let entry in iterator) {
> > +        File.remove(entry.path);
> > +      }
> 
> If we can't remove a single file for some reason we should still try to
> remove the remaining.

Fixed.

> ::: toolkit/components/thumbnails/test/browser_thumbnails_storage.js
> @@ +21,5 @@
> >    yield clearHistory();
> >    yield createThumbnail();
> >  
> >    // Make sure Storage.copy() updates an existing file.
> >    PageThumbsStorage.copy(URL, URL_COPY);
> 
> Needs to be yield PageThumbsStorage.copy(URL, URL_COPY);
> 
> @@ +27,3 @@
> >    let mtime = copy.lastModifiedTime -= 60;
> >  
> >    PageThumbsStorage.copy(URL, URL_COPY);
> 
> Needs to be yield PageThumbsStorage.copy(URL, URL_COPY);

Done.

> 
> ::: toolkit/components/thumbnails/test/head.js
> @@ +89,5 @@
> >   */
> >  function whenLoaded(aElement, aCallback) {
> >    aElement.addEventListener("load", function onLoad() {
> >      aElement.removeEventListener("load", onLoad, true);
> > +    SimpleTest.executeSoon(aCallback || next);
> 
> (pfft)

Ok, ok :)

> @@ +212,5 @@
> >   * @param aURL The URL of the thumbnail's page.
> >   * @param [optional] aCallback
> >   *        Function to be invoked on completion.
> >   */
> > +function whenFileExists(aURL, aCallback = next, depth = 0) {
> 
> What's |depth| for?

Ah, sorry, leftover debugging code.

> 
> @@ +218,5 @@
> >    if (!thumbnailExists(aURL)) {
> >      callback = function () whenFileExists(aURL, aCallback);
> >    }
> >  
> >    executeSoon(callback || next);
> 
> That default value for aCallback is nice - we don't need the fallback here
> if we have that.

Good point.

Comment 33

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

Attachment: Moving all I/O off the main thread, v6

Applied your feedback.

Comment 34

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

Try: https://tbpl.mozilla.org/?tree=Try&rev=be06ac371470

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach Teller [:Yoric] from comment #34)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=be06ac371470

That's the wrong try link, isn't it? :)

Comment 36

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

Yeah, that might be :)
Try: https://tbpl.mozilla.org/?tree=Try&rev=9356307282cb

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 728262 [details] [diff] [review]
Moving all I/O off the main thread, v6

I think you may have attached patch v5 again? I don't see any changes.

Comment 38

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

Attachment: Moving all I/O off the main thread, v7

That one should be better.

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 729541 [details] [diff] [review]
Moving all I/O off the main thread, v7

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

Great, thank you. Try looks good, let's land this \o/.

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/f09e10680ee4

Comment 41

[Tim Taubert [:ttaubert] (inactive)]

Attachment: follow-up to not expire thumbnails implicitly in browser_thumbnails_expiration.js

Looks like we have a little intermittent failure related to thumbnails being expired in the background while we're playing with them:

https://tbpl.mozilla.org/?tree=Fx-Team&rev=f09e10680ee4

I expect some more tests to be vulnerable to this but I think we can fix them one after the other if they should start to fail.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f09e10680ee4

Comment 43

[Ben Bucksch (:BenB)]

This bug caused extension breakage, see bug 857201.

Comment 44

[Alex Keybl [:akeybl]]

This didn't end up making the cut for the user facing release notes - still a very important improvement though, just difficult to communicate.

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 730289 [details] [diff] [review]
follow-up to not expire thumbnails implicitly in browser_thumbnails_expiration.js

Moving to bug 858888.

Comment 46

[Alex]

Nightly good version 2013-03-27 (No freeze)
Nightly bad version 2013-03-28(Freeze open  uploads of Shared Windows Folders with Russian symbol)
..................................................................................................
And new version (Freeze open  uploads of Shared Windows Folders with Russian symbol)

https://bugzilla.mozilla.org/show_bug.cgi?id=1031236

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cb432984d5ce&tochange=962f5293f87f

Comment 47

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

Alex, are you sure you're posting this on the right bug?

Comment 48

[Alex]

(In reply to David Rajchenbach Teller [:Yoric] from comment #47)
> Alex, are you sure you're posting this on the right bug?

I am not a developer but viewing all fixes 2013-03-28. This is the only post similar to my problem. All corporate mail freeze when uploading files from the Windows Shared folder (Russian Symbol).A 1 year I can not find a solution. I need help please.

Demonstration problem video 1. Firefox 22 Freeze  open uploads of Shared Windows Folders with Russian symbol. + Process Monitor
http://youtu.be/jXBQ6OmKyeE

Demonstration problem video 2. Firefox 20 NO Freeze  open uploads of Shared Windows Folders with Russian symbol. 
Firefox 30 Freeze  open uploads of Shared Windows Folders with Russian symbol + Process Monitor
http://youtu.be/DHVRjFmDa8c