Closed Bug 801598 Opened 12 years ago Closed 10 years ago

Extract the communication mechanisms of OMT OS.File into its own module (PromiseWorker)

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: Yoric, Assigned: Yoric, Mentored)

References

(Blocks 3 open bugs)

Details

(Keywords: dev-doc-needed, Whiteboard: [Async:ready][lang=js][experience required])

Attachments

(4 files, 12 obsolete files)

(deleted), patch
Yoric
: review+
Details | Diff | Splinter Review
(deleted), patch
Yoric
: review+
Details | Diff | Splinter Review
(deleted), patch
Yoric
: review+
Details | Diff | Splinter Review
(deleted), patch
Yoric
: review+
Details | Diff | Splinter Review
Attached file Example usage of this spawned version of OS.File (obsolete) (deleted) —
Experience with porting the Thumbnails storage to OS.File seems to indicate that OS.File is used in two fashions for the same module: - in a custom thread (bug 794804), along with client-specific code (e.g. to remove all the files of a given directory that are older than some date, except files that appear in some list) - bug ; and, simultaneously - using the shared OS.File worker (bug 753768), to use the regular OS.File operations and the infrastructure of message passing and promises. Using both threads in the same module is rather fragile, as it introduces race conditions, plus it requires reimplementing some of the message-passing code of async OS.File. At the same time, we probably do not want to let any client add random code to the async OS.File thread, as this could cause impossible-to-debug slowdowns/lockups, What we could do is let clients spawn instances of async OS.File extended which their own code.
Attachment #671397 - Flags: feedback?(ttaubert)
Attachment #671397 - Flags: feedback?(nfroyd)
(In reply to David Rajchenbach Teller [:Yoric] from comment #0) > Using both threads in the same module is rather fragile, as it introduces > race conditions, plus it requires reimplementing some of the message-passing > code of async OS.File. At the same time, we probably do not want to let any > client add random code to the async OS.File thread, as this could cause > impossible-to-debug slowdowns/lockups, This paragraph confuses me, particularly the first sentence--it sounds like you're saying using OS.File from multiple threads is fragile, which I hope is not the case! And we have APIs to let client code run on the OS.File thread? That sounds insane. Could you please explain this in more detail?
Let me rephrase. I mean that: - using two distinct threads to touch the same files is fragile; - the official async OS.File thread offers all the necessary architecture to pass instructions and results/exceptions back and forth (which is good), but none of the mechanism to execute client code (which seems reasonable); - the thread spawned for the PageThumbsStorage needs to reimplement some of this architecture for instructions/results/exceptions (which is error-prone), but can execute client code (which is the only reason to spawn it in the first place). If we introduce the ability to launch a thread that contains both the messaging architecture and the ability to run client code, we will be able to get rid of these potential race conditions. Which looks like the best way to implement e.g. bug 753768.
Comment on attachment 671397 [details] Example usage of this spawned version of OS.File I'm not a fan. I know that it's better to move your code to where the data is, rather than serializing the data and sending it over some channel, but that sort of action/executor architecture is not what we designed OS.File to do. I mean, if we're going down this path, why didn't we make everything a sync api that you must execute on a separate worker thread? Did we investigate to see if there's anything that can make the message-passing faster in the first place?
Attachment #671397 - Attachment mime type: application/x-javascript → text/plain
Attachment #671397 - Flags: feedback?(nfroyd)
Well, as you may recall, we do have a sync api that can be executed on a separate worker thread (see WIP doc at https://developer.mozilla.org/en-US/docs/JavaScript_OS.File ). In the case of bug 794804, we have a possibly very large directory, and walking it asynchronously + collecting required data requires 2 to 4 complex messages per entry. If the directory contains thousands of entries, this is the kind of thing that will have a very clear impact. By contrast, doing everything on its own thread reduces the number of messages to ~2. I am pretty sure that the same kind of issues can creep in with Sync, with cache-related bugs, and with Adblock – I actually had a discussion with Wladimir Palent in which he mentioned how communication costs basically killed his attempt to put code on a worker.
After discussing this with froydnj, we concluded that there is another, simpler method to achieve the same purpose: we just need to refactor OS.File to make the communication mechanism a public API. Rescoping this bug.
Assignee: nobody → dteller
Blocks: 753768
Summary: [OS.File] Spawn new customizable OS.File threads → [OS.File] Extract the communication mechanisms of OMT OS.File into its own module
Comment on attachment 671397 [details] Example usage of this spawned version of OS.File Removing feedback? as the bug's scope changed since the request and will require a new API.
Attachment #671397 - Flags: feedback?(ttaubert)
Not working actively on this. If someone is interested, please feel free to grab this bug.
Assignee: dteller → nobody
Component: OS.File → Async Tooling
Summary: [OS.File] Extract the communication mechanisms of OMT OS.File into its own module → Extract the communication mechanisms of OMT OS.File into its own module
Whiteboard: [Async]
Severity: normal → minor
Whiteboard: [Async] → [Async:ready][mentor=Yoric][lang=js][mentored but not simple]
Mentor: dteller
Whiteboard: [Async:ready][mentor=Yoric][lang=js][mentored but not simple] → [Async:ready][lang=js][mentored but not simple]
Whiteboard: [Async:ready][lang=js][mentored but not simple] → [Async:ready][lang=js][experience required]
Since bug 1024382 and the differential Updates mechanism for SessionWorker are going to cause lots of refactoring on the SessionWorker, I figured it's the right time to bring the error reporting of SessionWorker up to level with that of OS.File – and for this, to start sharing the code instead of using a copy & paste of an old version of OS.File.
Assignee: nobody → dteller
Attachment #671397 - Attachment is obsolete: true
Attachment #8444775 - Flags: review?(nfroyd)
Attachment #8444775 - Attachment description: Exposing communications mechanism into its own API → 1. Exposing communications mechanism into its own API
Attached patch 2. Applying to OS.File itself (obsolete) (deleted) — Splinter Review
This simplifies nicely the code of OS.File itself.
Attachment #8444778 - Flags: review?(nfroyd)
Attached patch 3. Applying to Thumbnails (obsolete) (deleted) — Splinter Review
Attachment #8444780 - Flags: review?(ttaubert)
Attached patch 4. Applying to Session Restore (obsolete) (deleted) — Splinter Review
Attachment #8444782 - Flags: review?(ttaubert)
I have double-checked – all the oranges above are due to bug 1016387, which I had in my mq for sanity checking.
Comment on attachment 8444780 [details] [diff] [review] 3. Applying to Thumbnails Review of attachment 8444780 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/thumbnails/PageThumbs.jsm @@ +792,5 @@ > /** > * Interface to a dedicated thread handling I/O > */ > +let PageThumbsWorker = { > + __proto__: new AbstractPromiseWorker("resource://gre/modules/PageThumbsWorker.js"), __proto__ is deprecated. Better: let aw = new AbstractPromiseWorker("resource://gre/modules/PageThumbsWorker.js"); let PageThumbsWorker = Object.create(aw, { log: { value: function () { // No logging supported. } }, }); (I *think* I got that right.) let PageThumbsWorker = Object.create(aw); PageThumbsWorker.log = function () { // No logging supported. }; That works too of course. I think I like that more. @@ +795,5 @@ > +let PageThumbsWorker = { > + __proto__: new AbstractPromiseWorker("resource://gre/modules/PageThumbsWorker.js"), > + log: function() { > + // No logging supported > + } Can this be provided by the AbstractPromiseWorker? As a no-op? ::: toolkit/components/thumbnails/PageThumbsWorker.js @@ +18,5 @@ > let File = OS.File; > let Type = OS.Shared.Type; > > +let worker = { > + __proto__: new PromiseWorker.AbstractWorker(), Same here about __proto__ being deprecated. @@ +24,5 @@ > + return Agent[method](...args); > + }, > + log: function(...args) { > + // No logging > + }, Can this be provided by the abstract worker? @@ +27,5 @@ > + // No logging > + }, > + postMessage: function(message, ...transfers) { > + self.postMessage(message, ...transfers); > + }, Can't this too be provided by the abstract worker? @@ +34,1 @@ > } Same here?
Attachment #8444780 - Flags: review?(ttaubert)
Comment on attachment 8444782 [details] [diff] [review] 4. Applying to Session Restore Review of attachment 8444782 [details] [diff] [review]: ----------------------------------------------------------------- See comments for previous patch.
Attachment #8444782 - Flags: review?(ttaubert)
Attached patch 1a Cumulative diff (obsolete) (deleted) — Splinter Review
Small addition to 1, as per ttaubert's feedback.
Attachment #8445130 - Flags: review?(nfroyd)
Attached patch 2. Applying to OS.File itself, v2 (obsolete) (deleted) — Splinter Review
Applied Tim's feedback.
Attachment #8444778 - Attachment is obsolete: true
Attachment #8444778 - Flags: review?(nfroyd)
Attachment #8445131 - Flags: review?(nfroyd)
Attached patch 3. Applying to Thumbnails, v2 (obsolete) (deleted) — Splinter Review
Applied feedback.
Attachment #8444780 - Attachment is obsolete: true
Attachment #8445133 - Flags: review?(ttaubert)
Attached patch 4. Applying to Session Restore, v2 (obsolete) (deleted) — Splinter Review
Attachment #8444782 - Attachment is obsolete: true
Comment on attachment 8445133 [details] [diff] [review] 3. Applying to Thumbnails, v2 Review of attachment 8445133 [details] [diff] [review]: ----------------------------------------------------------------- That's better :)
Attachment #8445133 - Flags: review?(ttaubert) → review+
Attachment #8445130 - Flags: review?(nfroyd) → review+
Comment on attachment 8444775 [details] [diff] [review] 1. Exposing communications mechanism into its own API Review of attachment 8444775 [details] [diff] [review]: ----------------------------------------------------------------- Either I'm confused about how |options| works, or we don't have tests for things. I'd really like to figure out how to not handle OS.File errors in the base PromiseWorker...can we add another function to the required API? ::: toolkit/components/promiseworker/PromiseWorker.jsm @@ +16,5 @@ > */ > > "use strict"; > > +this.EXPORTED_SYMBOLS = ["AbstractPromiseWorker"]; I think you need to make the naming between this patch and later patches for this class consistent. (BasePromiseWorker seems to be what later parts want it to be.) @@ +225,5 @@ > */ > + post: function(fun, args, options, closure) { > + return Task.spawn(function* postMessage() { > + let id = ++this._id; > + let message = {fun: fun, args: args, id: id}; It looks like we're not sending options to the worker here? The worker is still expecting |options|, AFAICS. @@ +276,5 @@ > + if (typeof options !== "object" || > + !("outExecutionDuration" in options)) { > + return reply.ok; > + } > + // If data.durationMs is not present, return data.ok (there was an s/data/reply/g, I think. ::: toolkit/components/promiseworker/moz.build @@ +6,5 @@ > + > +PARALLEL_DIRS += [ > + 'worker' > +] > + Nit: whitespace. ::: toolkit/components/promiseworker/worker/PromiseWorker.js @@ +20,5 @@ > +if (typeof Components != "undefined") { > + throw new Error("This module is meant to be used from the worker thread"); > +} > +if (typeof require == "undefined" || typeof module == "undefined") { > + throw new Error("this module is meant to be imported using the implementation fo require() at resource://gre/modules/workers/require.js"); Typo: "implementation of require" @@ +142,5 @@ > + } else { > + this.postMessage({ok: result, id:id, durationMs: durationMs}); > + } > + } else if (exn instanceof SharedAll.OSError) { > + this.log("Sending back OS.File error", exn, "id is", id); This seems a little weird to have this special-cased for OS.File in the base class. @@ +167,5 @@ > + this.postMessage({fail: error, id: id, durationMs: durationMs}); > + } else { > + // Other exceptions do not, and should be propagated through DOM's > + // built-in mechanism for uncaught errors, although this mechanism > + // may lose interesting information. Can you provide comments for the EXCEPTION_NAMES and StopIteration cases; this comment makes it sounds like we have just inserted special handling for those cases on a whim. ::: toolkit/components/promiseworker/worker/moz.build @@ +4,5 @@ > +# License, v. 2.0. If a copy of the MPL was not distributed with this > +# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + > +JS_MODULES_PATH = 'modules/workers' > + Nit: whitespace.
Attachment #8444775 - Flags: review?(nfroyd) → feedback+
Comment on attachment 8445131 [details] [diff] [review] 2. Applying to OS.File itself, v2 Review of attachment 8445131 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/osfile/modules/osfile_async_front.jsm @@ +379,5 @@ > * must be clonable. > * @return {Promise} A promise conveying the result/error caused by > * calling |method| with arguments |args|. > */ > + post: function post(method, args = undefined, closure = undefined) { This calling convention change ought to require a lot more updates somewhere?
Attachment #8445131 - Flags: review?(nfroyd)
(In reply to Nathan Froyd (:froydnj) from comment #21) > I'd really like to figure out how to not handle OS.File errors in the base > PromiseWorker...can we add another function to the required API? I'll see what I can do. > I think you need to make the naming between this patch and later patches for > this class consistent. (BasePromiseWorker seems to be what later parts want > it to be.) Ah, there seems to be something wrong with my qref, because on my computer, it's BasePromiseWorker. > It looks like we're not sending options to the worker here? The worker is > still expecting |options|, AFAICS. Well, these are options for the sending. If options need to be sent to the worker, the caller is in charge of putting them in `args` (which is the case in OS.File). > This seems a little weird to have this special-cased for OS.File in the base > class. Right. I'll see what I can do. (In reply to Nathan Froyd (:froydnj) from comment #22) > ::: toolkit/components/osfile/modules/osfile_async_front.jsm > @@ +379,5 @@ > > * must be clonable. > > * @return {Promise} A promise conveying the result/error caused by > > * calling |method| with arguments |args|. > > */ > > + post: function post(method, args = undefined, closure = undefined) { > > This calling convention change ought to require a lot more updates somewhere? Actually, no, it's just a cleanup, but it doesn't change the calling convention.
Applied feedback.
Attachment #8444775 - Attachment is obsolete: true
Attachment #8445130 - Attachment is obsolete: true
Attachment #8445435 - Flags: review?(nfroyd)
Attached patch 2. Applying to OS.File itself, v3 (obsolete) (deleted) — Splinter Review
Propagated changes.
Attachment #8445131 - Attachment is obsolete: true
Attachment #8445445 - Flags: review?(nfroyd)
Attachment #8445435 - Flags: review?(nfroyd) → review+
Attachment #8445445 - Flags: review?(nfroyd) → review+
Clarified comments. Reverted to old mechanism for passing `options`.
Attachment #8445435 - Attachment is obsolete: true
Attachment #8447676 - Flags: review+
Propagated changes.
Attachment #8445445 - Attachment is obsolete: true
Attachment #8447677 - Flags: review+
Attached patch 4. Applying to Session Restore, v3 (obsolete) (deleted) — Splinter Review
Fixed typo. Tim, it's not clear to me whether you wanted to re-review this patch.
Attachment #8445134 - Attachment is obsolete: true
Attachment #8447678 - Flags: review?(ttaubert)
Attached patch 3. Applying to Thumbnails, v3 (deleted) — Splinter Review
Fixed typo.
Attachment #8445133 - Attachment is obsolete: true
Attachment #8447679 - Flags: review+
Comment on attachment 8447678 [details] [diff] [review] 4. Applying to Session Restore, v3 Review of attachment 8447678 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionFile.jsm @@ +269,5 @@ > // write instructions. > isFinalWrite = this._isClosed = true; > } > > + let deferWritten = Promise.defer(); Nit: maybe |deferredWritten| as it returns a Deferred object? @@ +276,5 @@ > > + // Ensure that we can write sessionstore.js cleanly before the profile > + // becomes unaccessible. > + AsyncShutdown.profileBeforeChange.addBlocker( > + "SessionFile: Finish writing the sessionstore.js", Nit: either "writing sessionstore.js" or "the sessionstore.js file" :) ::: browser/components/sessionstore/src/SessionWorker.jsm @@ +18,5 @@ > > this.EXPORTED_SYMBOLS = ["SessionWorker"]; > > +this.SessionWorker = new BasePromiseWorker("resource:///modules/sessionstore/SessionWorker.js"); > +// As the PageThumbsWorker performs I/O, we can receive instances of s/PageThumbsWorker/SessionWorker
Attachment #8447678 - Flags: review?(ttaubert) → review+
Applied feedback.
Attachment #8447678 - Attachment is obsolete: true
Attachment #8448689 - Flags: review+
(In reply to Carsten Book [:Tomcat] from comment #34) > https://hg.mozilla.org/integration/mozilla-inbound/rev/781fc1eb104a Hey Yoric, had to back this out for https://tbpl.mozilla.org/php/getParsedLog.php?id=42917674&tree=Mozilla-Inbound - is this a problem of this checkin or because i failed and only checkedin this one csets where the others had to land too ?
All the csets need to land together. Otherwise, several modules will fail to communicate with their threads, which would be a shame.
Depends on: 1033972
Summary: Extract the communication mechanisms of OMT OS.File into its own module → Extract the communication mechanisms of OMT OS.File into its own module (PromiseWorker)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: