Closed Bug 777711 Opened 12 years ago Closed 12 years ago

[OS.File] Async API without finalization

Categories

(Toolkit Graveyard :: OS.File, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: dev-doc-needed)

Attachments

(4 files, 20 obsolete files)

21.65 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
1.21 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
19.55 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
8.55 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
We should first provide a first version of the async OS.File API (main thread API with actual I/O deferred to a background-thread) without auto-closing of leaking file descriptors.
Attached patch Companion makefile (obsolete) — Splinter Review
Assignee: nobody → dteller
Attached patch 1. Main thread module (obsolete) — Splinter Review
Attaching a first version of the main thread code. It still contains some logging code and there are dependencies to unreviewed code, but I would be interested in some feedback.
Attachment #647170 - Flags: feedback?(taras.mozilla)
Attached patch 2. Off-main-thread worker (obsolete) — Splinter Review
Attachment #647171 - Flags: feedback?(taras.mozilla)
Attachment #647171 - Attachment is patch: true
Attached patch 3. Companion testsuite (obsolete) — Splinter Review
Attachment #647172 - Flags: feedback?(taras.mozilla)
Comment on attachment 647170 [details] [diff] [review]
1. Main thread module

2 good things about this patchset:
a) numbered patches
b) most questions I have seem to be answered by comments

+      let handler = self._queue.shift();
This could get bad O(n^2) with a lot of outstanding io operations. Should abstract that away into a queue data structure so we can optimize it later.

+    worker.onmessage = function onmessage(msg) {
+        deferred.resolve(data.ok);
+      } else if ("ko" in data) {

call it "fail" or "error"

It's a little worrying that there is no operation field to check that the operation in the queue matches the response we got...would be terrible to get out of sync in this case, is there a safeguard against that?

+      let id = handler.id;  <-- this id isn't used for anything like that, isn't it mean to?


+    LOG("Posting message", JSON.stringify(message));
I think this is too generic. We should define a stricter c-style api of
[ id, payload_byte1,payload_byte2] style and use js arrays or arraybuffers or whatever makes sense.

we should also avoid stringifying given:

Note: Prior to Gecko 6.0 (Firefox 6.0 / Thunderbird 6.0 / SeaMonkey 2.3) , the message parameter must be a string. Starting in Gecko 6.0 (Firefox 6.0 / Thunderbird 6.0 / SeaMonkey 2.3) , the message parameter is serialized using the structured clone algorithm. This means you can pass a broad variety of data objects safely to the destination window without having to serialize them yourself

Also in the future we'll have 0-copy transferable so we can optimize it further: http://www.whatwg.org/specs/web-apps/current-work/multipage/common-dom-interfaces.html#transferable-objects

Note we should do minimal work here atm, just make sure that we write the api in such a way that above optimizations are not made impossible. For now the key is to get rid of stringify and rely on structured clones to minimize overhead.

+   * @type {array<{deferred:deferred, root:*=}>} 
s/root/closure/

s/@rejects/@throws/g

for the promise-uninitiated of us,what does 
+    return Scheduler.post(["File_prototype_stat", this._fdmsg], this).then(
+      File.Info.fromMsg
+    );
do? Does that mean that File.Info.fromMsg transforms return value on promise fulfillment?

+    // Note: Classic semantics for ArrayBuffer communication would imply
+    // that posting the ArrayBuffer removes ownership from the sender
+    // thread. Here, we use Type.voidptr_t.toMsg to ensure that these
+    // semantics do not apply.
Isn't that OK? Wouldn't ownership come back when the message comes back?
void_t cast is to make sure we avoid memcpys? that's cool JS :)

+  readAll: function readAll(buffer, nbytes, options) {
readAll seems like somethign that should be implemented on the worker-side. No point in wasting ipc traffic here, right?

writeAll too


looks good overall.

One thing to consider is in the future is 
readWith(transformer) which would uneval(or something to send the function to worker), then do any transformations in the worker to reduce amount of processing needed on main thread.
Attachment #647170 - Flags: feedback?(taras.mozilla) → feedback+
Comment on attachment 647170 [details] [diff] [review]
1. Main thread module



+// The library of promises.
+// FIXME: For the moment, we embed a copy of toolkit/addon-sdk's implementation
+// of promises, until this library is properly exported.
+let Promise = {};
+Components.utils.import("resource://gre/modules/promise.jsm", Promise);

Can you elaborate on what's going on and what the ramifications of this todo are?
Comment on attachment 647171 [details] [diff] [review]
2. Off-main-thread worker

+ let method = data.val.shift();
Should atleast change the protocol to pop from arrays rather than shift



+         gFile._setHandle(fd);
That's quite a hack :)

Should _setHandle(fd)  before calling
+ result = Agent[method].apply(Agent, data.val);
and then immediately _setHandle(-1) to make sure the handle doesn't accidentally persist and we call an io op on a handle we didn't mean to call it on
Attachment #647171 - Flags: feedback?(taras.mozilla) → feedback+
Comment on attachment 647172 [details] [diff] [review]
3. Companion testsuite

I like how this bug is turning out. This api is going to be a whole lot nicer (and actually async) than what we have now.

But Tim should also OK this bug.
Attachment #647172 - Flags: feedback?(ttaubert)
Attachment #647172 - Flags: feedback?(taras.mozilla)
Attachment #647172 - Flags: feedback+
Comment on attachment 647172 [details] [diff] [review]
3. Companion testsuite

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

That API looks nice!
Attachment #647172 - Flags: feedback?(ttaubert) → feedback+
(In reply to Taras Glek (:taras) from comment #6)
> Comment on attachment 647170 [details] [diff] [review]
> 1. Main thread module
> 
> 
> 
> +// The library of promises.
> +// FIXME: For the moment, we embed a copy of toolkit/addon-sdk's
> implementation
> +// of promises, until this library is properly exported.
> +let Promise = {};
> +Components.utils.import("resource://gre/modules/promise.jsm", Promise);
> 
> Can you elaborate on what's going on and what the ramifications of this todo
> are?

Essentially, this is a workaround for bug 774816 not having landed. At the moment, the code for promises is on m-c but there is no Makefile support to make it available for users. I have a hack in the Makefile that publishes the file as "resource://gre/modules/promise.jsm" and I use this file here.

I hope that bug 774816 lands soon. Otherwise, I can clean up this hack a little.
(In reply to Taras Glek (:taras) from comment #5)
> Comment on attachment 647170 [details] [diff] [review]
> 1. Main thread module
> 
> 2 good things about this patchset:
> a) numbered patches
> b) most questions I have seem to be answered by comments
> 
> +      let handler = self._queue.shift();
> This could get bad O(n^2) with a lot of outstanding io operations. Should
> abstract that away into a queue data structure so we can optimize it later.

Will do.

> +    worker.onmessage = function onmessage(msg) {
> +        deferred.resolve(data.ok);
> +      } else if ("ko" in data) {
> 
> call it "fail" or "error"

Ok.

> 
> It's a little worrying that there is no operation field to check that the
> operation in the queue matches the response we got...would be terrible to
> get out of sync in this case, is there a safeguard against that?
> 
> +      let id = handler.id;  <-- this id isn't used for anything like that,
> isn't it mean to?

It is meant in case we want to, so no problem.

> +    LOG("Posting message", JSON.stringify(message));
> I think this is too generic. We should define a stricter c-style api of
> [ id, payload_byte1,payload_byte2] style and use js arrays or arraybuffers
> or whatever makes sense.
> 
> we should also avoid stringifying given:

I am just stringifying the debug messages, because it gives me more interesting data than toSource or toString, nothing else.

[...]

> Note we should do minimal work here atm, just make sure that we write the
> api in such a way that above optimizations are not made impossible. For now
> the key is to get rid of stringify and rely on structured clones to minimize
> overhead.

That's what I do.

> 
> +   * @type {array<{deferred:deferred, root:*=}>} 
> s/root/closure/

Ok
 
> s/@rejects/@throws/g

No, @throws means that the function throws an exception. Here, it always returns a promise, but the promise may end up in a resolved state or in a rejected state. This is the official vocabulary of promise, as well as the name of methods in the implementation of promises, so I introduced a new pseudo-jsdoc flag for this purpose.

> 
> for the promise-uninitiated of us,what does 
> +    return Scheduler.post(["File_prototype_stat", this._fdmsg], this).then(
> +      File.Info.fromMsg
> +    );
> do? Does that mean that File.Info.fromMsg transforms return value on promise
> fulfillment?

In case of promise fulfillment, this returns the result of applying |File.Info.fromMsg| to the result of the promise and returns the corresponding promise.

> 
> +    // Note: Classic semantics for ArrayBuffer communication would imply
> +    // that posting the ArrayBuffer removes ownership from the sender
> +    // thread. Here, we use Type.voidptr_t.toMsg to ensure that these
> +    // semantics do not apply.
> Isn't that OK? Wouldn't ownership come back when the message comes back?

Well, I could not find any clear specifications on the topic, but exchanges on the w3c mailing-lists lead me to believe that no, ownership would not come back.

> void_t cast is to make sure we avoid memcpys? that's cool JS :)

:)

> 
> +  readAll: function readAll(buffer, nbytes, options) {
> readAll seems like somethign that should be implemented on the worker-side.
> No point in wasting ipc traffic here, right?
> 
> writeAll too

I was wondering about that. I have a worker-side implementation of readAll, too, but I figured that if read does not return as many bytes as we would like it to, it might be because of a serious disk (or remote file system) slowdown. Placing this controller-side does waste ipc traffic but ensures that we are not starving for this IO.

Not sure which choice is best.

> looks good overall.
> 
> One thing to consider is in the future is 
> readWith(transformer) which would uneval(or something to send the function
> to worker), then do any transformations in the worker to reduce amount of
> processing needed on main thread.

Yes, I have a few notes on how to best do this. No clear conclusion yet, though.
(In reply to Taras Glek (:taras) from comment #7)
> Comment on attachment 647171 [details] [diff] [review]
> 2. Off-main-thread worker
> 
> + let method = data.val.shift();
> Should atleast change the protocol to pop from arrays rather than shift

Ok, next version will use an efficient O(n·ln(n)) implementation of queues :)

> +         gFile._setHandle(fd);
> That's quite a hack :)
> 
> Should _setHandle(fd)  before calling
> + result = Agent[method].apply(Agent, data.val);
> and then immediately _setHandle(-1) to make sure the handle doesn't
> accidentally persist and we call an io op on a handle we didn't mean to call
> it on

Good idea.
(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #12)
> (In reply to Taras Glek (:taras) from comment #7)
> > Comment on attachment 647171 [details] [diff] [review]
> > 2. Off-main-thread worker
> > 
> > + let method = data.val.shift();
> > Should atleast change the protocol to pop from arrays rather than shift
> 
> Ok, next version will use an efficient O(n·ln(n)) implementation of queues :)

Please don't. You do not need a queue here, just change message structure so it doesn't require a shift.
(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from 
> > s/@rejects/@throws/g
> 
> No, @throws means that the function throws an exception. Here, it always
> returns a promise, but the promise may end up in a resolved state or in a
> rejected state. This is the official vocabulary of promise, as well as the
> name of methods in the implementation of promises, so I introduced a new
> pseudo-jsdoc flag for this purpose.

Sure, that's ok but you need to document that in the file.

Anyway, just to be clear. Lets leave efficient queues as a followup, I just want the queue to be abstracted out and implemented with Array.shift as it is now.

The way the server-side is written now, I'm not sure you'd have much IO outstanding.

The other thing that's kind of weird if how you pass file handles back to the main thread. 
Make a dictionary of worker-side-id->filehandle. This way we'll still get finalization behavior when the worker dies. And then instead of swapping the underlying fd, just swap gfile.
(In reply to Taras Glek (:taras) from comment #13)
> (In reply to David Rajchenbach Teller (away until early August) [:Yoric]
> from comment #12)
> > (In reply to Taras Glek (:taras) from comment #7)
> > > Comment on attachment 647171 [details] [diff] [review]
> > > 2. Off-main-thread worker
> > > 
> > > + let method = data.val.shift();
> > > Should atleast change the protocol to pop from arrays rather than shift
> > 
> > Ok, next version will use an efficient O(n·ln(n)) implementation of queues :)
> 
> Please don't. You do not need a queue here, just change message structure so
> it doesn't require a shift.

Got it.
(In reply to Taras Glek (:taras) from comment #14)
> (In reply to David Rajchenbach Teller (away until early August) [:Yoric]
> from 
> > > s/@rejects/@throws/g
> > 
> > No, @throws means that the function throws an exception. Here, it always
> > returns a promise, but the promise may end up in a resolved state or in a
> > rejected state. This is the official vocabulary of promise, as well as the
> > name of methods in the implementation of promises, so I introduced a new
> > pseudo-jsdoc flag for this purpose.
> 
> Sure, that's ok but you need to document that in the file.
> 
> Anyway, just to be clear. Lets leave efficient queues as a followup, I just
> want the queue to be abstracted out and implemented with Array.shift as it
> is now.

Ah, well, too late. I have a 15 lines implementation of Queue in O(n·log(n)) that should be rather efficient.

> The way the server-side is written now, I'm not sure you'd have much IO
> outstanding.

In the tests, so far, my queues have always contained between 0 and 2 items.

> 
> The other thing that's kind of weird if how you pass file handles back to
> the main thread. 
> Make a dictionary of worker-side-id->filehandle. This way we'll still get
> finalization behavior when the worker dies. And then instead of swapping the
> underlying fd, just swap gfile.

I don't understand the idea, could you elaborate? Barring any mistake, as long as I have a dictionary, I can't have finalization. Plus with the technique I use right now, I know how to perform detection of leaked file descriptors.
(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from > 
> I don't understand the idea, could you elaborate? Barring any mistake, as
> long as I have a dictionary, I can't have finalization. Plus with the
> technique I use right now, I know how to perform detection of leaked file
> descriptors.

Say you open 2 files. You then fail to close them,
then there are no more references to worker, it gets closed too. Then your worker's gFile will just happen to point to one of the fds....it'll get finalized, the other one will leak.


On the other if you maintain a proper handle for every worker in an array..and use array index as id to pass across threads...Then when the array gets gced, all of the entries in it get finalized.
(In reply to Taras Glek (:taras) from comment #17)
> Say you open 2 files. You then fail to close them,
> then there are no more references to worker, it gets closed too. Then your
> worker's gFile will just happen to point to one of the fds....it'll get
> finalized, the other one will leak.
> 
> 
> On the other if you maintain a proper handle for every worker in an
> array..and use array index as id to pass across threads...Then when the
> array gets gced, all of the entries in it get finalized.

That might work (I assume the array is in the worker, rather than on the controller). However, this requires that each API client requests one worker thread (or an abstraction thereof), uses this worker thread for I/O, and eventually stops referencing the worker thread. I suspect that this is not what we want:
- creating a worker thread is expensive (among other things, you need to instantiate a new JS environment and re-JIT everything);
- letting API clients spawn gazillions of worker threads encourages disk thrashing;
- I am pretty sure that most developers (including add-on developers) will use the simplest path, i.e. keep a global reference to the worker thread, hence preventing any garbage-collection.

So, while we can offer such an API, I would rather keep that for later and concentrate on the current design.

Note that the current design has advantages that this "one worker per client" does not have. By keeping the workers stateless, we can perform detection and reporting of file leaks on a per-file basis, without any concurrency issues. Also, by keeping the workers stateless, we effectively have a service that can trivially scale to use a pool of worker threads - and by controlling the pool ourselves, we can easily limit disk thrashing.

Note that, in the current state, gFile will not finalize anything (this is beyond the scope of this bug). However, I have on my local repo an experimental version of bug 777715 that not only finalizes leaked files but reports them to the user.
Attached patch 1. Main thread module, v2 (obsolete) — Splinter Review
New version of the main thread module, taking into consideration feedback:
- redesigned a little the protocol to avoid shift() and to replace "ko" with "fail";
- we now have a Queue object;
- additional sanity checks using |id|.

Also, added the following missing features:
- getPosition/setPosition;
- copy;
- move.
Attachment #647170 - Attachment is obsolete: true
Attachment #648297 - Flags: feedback?(taras.mozilla)
Attached patch 2. Off-main-thread worker, v2 (obsolete) — Splinter Review
Propagated the protocol changes and added the new functions.
Attachment #647171 - Attachment is obsolete: true
Attachment #648299 - Flags: feedback?(taras.mozilla)
Attached patch 3. Companion testsuite, v2 (obsolete) — Splinter Review
Testsuite for the new features.
Attachment #647172 - Attachment is obsolete: true
Attachment #648301 - Flags: feedback?(ttaubert)
Attachment #648301 - Flags: feedback?(taras.mozilla)
Attachment #648301 - Attachment is patch: true
I do not see why my suggestion would be one-worker-per-client. It'd be exactly the same, but you would not be passing around raw fds.
Comment on attachment 648297 [details] [diff] [review]
1. Main thread module, v2

f+, except that I find it hard to believe that this queue implementation is more efficient given the workload. You are basically allocating a new array on every push/pop...since the queue does not get deep I find it hard to imagine this being faster than the original .shift()-based queue. Instead of memmove()ing in C, you are exercising the GC a lot.
I think a simple Queue wrapper around array.shift will do until we know the queue gets sufficiently deep to warrant this.
Attachment #648297 - Flags: feedback?(taras.mozilla) → feedback+
Comment on attachment 648299 [details] [diff] [review]
2. Off-main-thread worker, v2

I think a table of descriptors will prevent a leak here. Otherwise this implementation is guaranteed to leak if close is not called.
Attachment #648299 - Flags: feedback?(taras.mozilla) → feedback-
Comment on attachment 648301 [details] [diff] [review]
3. Companion testsuite, v2

i don't tjink there is any point in f?ing the testcase again. It's good to have it as an example of how the api to be used, but does not need any further review from me.
Attachment #648301 - Flags: feedback?(taras.mozilla)
(In reply to Taras Glek (:taras) from comment #24)
> Comment on attachment 648299 [details] [diff] [review]
> 2. Off-main-thread worker, v2
> 
> I think a table of descriptors will prevent a leak here. Otherwise this
> implementation is guaranteed to leak if close is not called.

For the reasons mentioned in comment 18, I would rather not move towards an implementation that closes leaking file descriptors only when the thread is killed/garbage-collected. If I can show you an implementation that closes leaking file descriptors without changing the current design, will you let me continue with it?
(In reply to David Rajchenbach Teller from comment #26)
> (In reply to Taras Glek (:taras) from comment #24)
> > Comment on attachment 648299 [details] [diff] [review]
> > 2. Off-main-thread worker, v2
> > 
> > I think a table of descriptors will prevent a leak here. Otherwise this
> > implementation is guaranteed to leak if close is not called.
> 
> For the reasons mentioned in comment 18, I would rather not move towards an
> implementation that closes leaking file descriptors only when the thread is
> killed/garbage-collected. If I can show you an implementation that closes
> leaking file descriptors without changing the current design, will you let
> me continue with it?

That's what I'm asking for. Lets chat on skype first. i think we may be talking past each other here.
Ok. In the meantime, I have uploaded my implementation of finalization as part of bug 777715. As you may see, it requires the addition of about 2 loc in OS.File (and plenty of application-independent support code) and I believe that this is the way to go.
Comment on attachment 648301 [details] [diff] [review]
3. Companion testsuite, v2

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

Nice API you got there!
Attachment #648301 - Flags: feedback?(ttaubert) → feedback+
Attached patch 1. Main thread module, v3 (obsolete) — Splinter Review
Attachment #648297 - Attachment is obsolete: true
Comment on attachment 650530 [details] [diff] [review]
1. Main thread module, v3

Updated controller. No meaningful change on this side.
Attachment #650530 - Attachment is patch: true
Attached patch 2. Off-main-thread worker, v3 (obsolete) — Splinter Review
Attaching a new version of the worker. As requested, this new version keeps file descriptors on the worker thread. This ensures that leaked files are closed at *process* exit.

As mentioned elsewhere, this comes at the cost of testability, flexibility wrt thread pools, analyzing leaked files during execution (which I suspect would be a useful feature for add-on developers).
Attachment #648299 - Attachment is obsolete: true
Attachment #650532 - Flags: feedback?(taras.mozilla)
Attached patch 3. Companion testsuite, v3 (obsolete) — Splinter Review
No meaningful change here.
Attachment #648301 - Attachment is obsolete: true
Comment on attachment 650532 [details] [diff] [review]
2. Off-main-thread worker, v3

I do not understand what's going on in this if clause:
+           LOG("Result is positive", result, JSON.stringify(result),
Please explain.
Attachment #650532 - Flags: feedback?(taras.mozilla) → feedback+
(In reply to Taras Glek (:taras) from comment #34)
> Comment on attachment 650532 [details] [diff] [review]
> 2. Off-main-thread worker, v3
> 
> I do not understand what's going on in this if clause:
> +           LOG("Result is positive", result, JSON.stringify(result),
> Please explain.

Not sure which if clause (it would be great if Bugzilla could offer a presentation like that of github to attach comments to lines).

If this is the |if (typeof ...)|, this is just logging code, to ensure that we do not call |toSource()| on something that does not have a method |toSource()| (i.e. |null| or a number).

If this is the |if(!exn)|, well, this is because values and exceptions cannot be serialized in the same manner.
(In reply to David Rajchenbach Teller from comment #35)
> (In reply to Taras Glek (:taras) from comment #34)
> > Comment on attachment 650532 [details] [diff] [review]
> > 2. Off-main-thread worker, v3
> > 
> > I do not understand what's going on in this if clause:
> > +           LOG("Result is positive", result, JSON.stringify(result),
> > Please explain.
> 
> Not sure which if clause (it would be great if Bugzilla could offer a
> presentation like that of github to attach comments to lines).

The "result is positive" if clause. I'm not sure what that logging message means
(In reply to Taras Glek (:taras) from comment #36)
> (In reply to David Rajchenbach Teller from comment #35)
> > (In reply to Taras Glek (:taras) from comment #34)
> > > Comment on attachment 650532 [details] [diff] [review]
> > > 2. Off-main-thread worker, v3
> > > 
> > > I do not understand what's going on in this if clause:
> > > +           LOG("Result is positive", result, JSON.stringify(result),
> > > Please explain.
> > 
> > Not sure which if clause (it would be great if Bugzilla could offer a
> > presentation like that of github to attach comments to lines).
> 
> The "result is positive" if clause. I'm not sure what that logging message
> means

Does it really matter? It's debugging code that is most likely going to disappear by the time I set this "r?".

Now, |if (typeof(result) == "object")| determines if |result| is an object, in which case we can display its source code with |Object.prototype.toSource|. This has helped me once or twice catch errors.
Or perhaps you are asking how function |LOG| works?

LOG(arg1, arg2, ...)

simply dumps |someprefix + " " + arg1 + " " + arg2 + " " ... + "\n"| on stderr
Attached patch 1. Main thread module, v4 (obsolete) — Splinter Review
Now that the underlying API is basically stabilized, it is time to start the review process for async OS.File!

I plan to land this with the current set of features. While asynchronous iteration through a directory is already implemented, this will go into a followup bug.
Attachment #650530 - Attachment is obsolete: true
Attachment #652917 - Flags: review?(taras.mozilla)
Comment on attachment 652917 [details] [diff] [review]
1. Main thread module, v4

You forgot to move writeAll to the worker(like readAll). r+ with that fix
Attachment #652917 - Flags: review?(taras.mozilla) → review+
Attached patch 2. Off-main-thread worker, v4 (obsolete) — Splinter Review
Attachment #650532 - Attachment is obsolete: true
Attachment #653790 - Flags: review?(taras.mozilla)
Attached patch 3. Companion testsuite, v4 (obsolete) — Splinter Review
Added a few tests
Attachment #650534 - Attachment is obsolete: true
Attachment #653791 - Flags: review?(taras.mozilla)
Comment on attachment 653790 [details] [diff] [review]
2. Off-main-thread worker, v4

s/preference/pref :)
Attachment #653790 - Flags: review?(taras.mozilla) → review+
Attachment #653791 - Flags: review?(taras.mozilla) → review+
Attached patch 3. Companion testsuite, v5 (obsolete) — Splinter Review
Attachment #653791 - Attachment is obsolete: true
Attachment #656016 - Flags: review+
Attached patch 2. Off-main-thread worker, v5 (obsolete) — Splinter Review
Attachment #653790 - Attachment is obsolete: true
Attachment #656018 - Flags: review+
Attached patch 1. Main thread module, v5 (obsolete) — Splinter Review
Propagated the changes of bug 780483.
Attachment #652917 - Attachment is obsolete: true
Attachment #656024 - Flags: review+
Attached patch Companion makefile (obsolete) — Splinter Review
Attachment #647168 - Attachment is obsolete: true
Attachment #657163 - Flags: review+
Attached patch 1. Main thread module, v5 (obsolete) — Splinter Review
Attachment #656024 - Attachment is obsolete: true
Attachment #657164 - Flags: review+
Attached patch 2. Off-main-thread worker, v6 (obsolete) — Splinter Review
Attachment #656018 - Attachment is obsolete: true
Attachment #657165 - Flags: review+
Attached patch 3. Companion testsuite, v6 (obsolete) — Splinter Review
Attaching latest version of code (updated to comply with the changes to parent patches), in case I need to disappear for two weeks without warning.
Attachment #656016 - Attachment is obsolete: true
Attachment #657167 - Flags: review+
(merging)
Attachment #657167 - Attachment is obsolete: true
Attachment #663745 - Flags: review+
Attachment #657163 - Attachment is obsolete: true
Attachment #663746 - Flags: review+
(merging)
Attachment #657164 - Attachment is obsolete: true
Attachment #663747 - Flags: review+
(merging)
Attachment #657165 - Attachment is obsolete: true
Attachment #663748 - Flags: review+
The last Try push shows failures, no?
Keywords: checkin-needed
Depends on: 885615
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: