The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Tabbed Browser
--
enhancement
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

unspecified
Firefox 22
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(relnote-firefox -)

Details

Attachments

(1 attachment, 14 obsolete attachments)

35.65 KB, patch
ttaubert
: review+
Details | Diff | Splinter Review
At the moment Page Thumbnails custom storage uses NetUtils + xpcom.

For better performance, we should port it to off-main thread OS.File.
Created attachment 623654 [details] [diff] [review]
First prototype

Attaching a first draft.
Assignee: nobody → dteller
Status: NEW → ASSIGNED
Created attachment 628741 [details] [diff] [review]
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.
Attachment #623654 - Attachment is obsolete: true
Attachment #628741 - Flags: feedback?(ttaubert)
Created attachment 630483 [details] [diff] [review]
Implementing asynchronous |write|, |copy|, |remove|.

All tests seem to pass, except browser_thumbnails_bug753755 – which I suspect did not pass either before my patch.
Attachment #628741 - Attachment is obsolete: true
Attachment #628741 - Flags: feedback?(ttaubert)
Attachment #630483 - Flags: review?(ttaubert)
Comment on attachment 630483 [details] [diff] [review]
Implementing asynchronous |write|, |copy|, |remove|.

Ironing out a few issues.
Attachment #630483 - Flags: review?(ttaubert)
Created attachment 630990 [details] [diff] [review]
Implementing asynchronous |write|, |copy|, |remove|.
Attachment #630483 - Attachment is obsolete: true
Attachment #630990 - Flags: review?(ttaubert)
Created attachment 630991 [details] [diff] [review]
Companion testsuite
Attachment #630991 - Flags: review?(ttaubert)
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"?
Depends on: 754671
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.
Attachment #630990 - Flags: review?(ttaubert)
Attachment #630991 - Flags: review?(ttaubert)
Depends on: 772538
Depends on: 707694
Depends on: 781346
No longer depends on: 707694, 772538
Summary: [Page thumbnails] Port storage to OS.File → [Page thumbnails] Use OS.File to write and copy
Created attachment 670387 [details] [diff] [review]
Implementing asynchronous |write|, |copy|.
Attachment #630990 - Attachment is obsolete: true
Attachment #630991 - Attachment is obsolete: true
Attachment #670387 - Flags: feedback?(ttaubert)
Depends on: 801598
Created attachment 685191 [details] [diff] [review]
Moving all I/O off the main thread
Attachment #670387 - Attachment is obsolete: true
Attachment #670387 - Flags: feedback?(ttaubert)
Attachment #685191 - Flags: review?(ttaubert)
Depends on: 815339
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.
(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.
(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?
(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?
Summary: [Page thumbnails] Use OS.File to write and copy → [Page thumbnails] Move all I/O off the main thread
Created attachment 687369 [details] [diff] [review]
Moving all I/O off the main thread, v2
Attachment #685191 - Attachment is obsolete: true
Attachment #685191 - Flags: review?(ttaubert)
Attachment #687369 - Flags: review?(ttaubert)
Created attachment 687379 [details] [diff] [review]
Moving all I/O off the main thread, v3

I have applied your feedback and taken the opportunity to fix deserialization of exceptions.
Attachment #687379 - Flags: review?(ttaubert)
(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.
(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?
Attachment #687369 - Attachment is obsolete: true
Attachment #687369 - Flags: review?(ttaubert)
Comment on attachment 687379 [details] [diff] [review]
Moving all I/O off the main thread, v3

Oops.
Attachment #687379 - Attachment description: Moving all I/O off the main thread → Moving all I/O off the main thread, v3
Review ping?
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.
Attachment #687379 - Flags: review?(ttaubert) → feedback+
(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 :)
Created attachment 693839 [details] [diff] [review]
Moving all I/O off the main thread, v3

Same code, with feedback applied, and now using localProfileDir.
Attachment #687379 - Attachment is obsolete: true
Attachment #693839 - Flags: review?(ttaubert)
(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 :)
(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.
(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
Created attachment 693861 [details] [diff] [review]
Moving all I/O off the main thread, v4

Here we go.
Attachment #693839 - Attachment is obsolete: true
Attachment #693839 - Flags: review?(ttaubert)
Attachment #693861 - Flags: review?(ttaubert)
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).
Attachment #693861 - Flags: review?(ttaubert) → feedback+
(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.
Created attachment 725007 [details] [diff] [review]
Moving all I/O off the main thread, v5
Attachment #693861 - Attachment is obsolete: true
Attachment #725007 - Flags: review?(ttaubert)
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.
Attachment #725007 - Flags: review?(ttaubert) → feedback+
Blocks: 841495
(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.
Created attachment 728262 [details] [diff] [review]
Moving all I/O off the main thread, v6

Applied your feedback.
Attachment #725007 - Attachment is obsolete: true
Attachment #728262 - Flags: review?(ttaubert)
Try: https://tbpl.mozilla.org/?tree=Try&rev=be06ac371470
(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? :)
Yeah, that might be :)
Try: https://tbpl.mozilla.org/?tree=Try&rev=9356307282cb
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.
Attachment #728262 - Flags: review?(ttaubert)
Created attachment 729541 [details] [diff] [review]
Moving all I/O off the main thread, v7

That one should be better.
Attachment #728262 - Attachment is obsolete: true
Attachment #729541 - Flags: review?(ttaubert)
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/.
Attachment #729541 - Flags: review?(ttaubert) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/f09e10680ee4
Keywords: checkin-needed
Created attachment 730289 [details] [diff] [review]
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.
Attachment #730289 - Flags: review?(dteller)
https://hg.mozilla.org/mozilla-central/rev/f09e10680ee4
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
relnote-firefox: --- → ?

Comment 43

4 years ago
This bug caused extension breakage, see bug 857201.
This didn't end up making the cut for the user facing release notes - still a very important improvement though, just difficult to communicate.
relnote-firefox: ? → -
Comment on attachment 730289 [details] [diff] [review]
follow-up to not expire thumbnails implicitly in browser_thumbnails_expiration.js

Moving to bug 858888.
Attachment #730289 - Attachment is obsolete: true
Attachment #730289 - Flags: review?(dteller)

Comment 46

3 years ago
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
Alex, are you sure you're posting this on the right bug?

Comment 48

3 years ago
(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
You need to log in before you can comment on or make changes to this bug.