Bug 1061288 will make it impossible to convert ArrayBuffer to C pointer and pass it across workers. Bug 1075438 should remove the only reason for which we do this in OS.File. All that is left is reworking OS.File and make sure that we never do it.

Note that this will not be sufficient to get rid of all pointer hackery, but there is a followup patch in the blocked bug that should get rid of the rest.

Ah, forgot to fix a test. Here we go.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=be0d222df0da

Comment on attachment 8525980 [details] [diff] [review] 1. New manner to specify transferables to a PromiseWorker Review of attachment 8525980 [details] [diff] [review]: ----------------------------------------------------------------- So we're doing this so that individual objects that we send can flag themselves as transferable without the sender needing to know anything about transferability, correct? Do we even need the explicit |transfers| argument anymore? ::: toolkit/components/promiseworker/PromiseWorker.jsm @@ +386,5 @@ > + * @constructor > + */ > +BasePromiseWorker.Meta = function(data, meta) { > + this.data = data; > + this.meta = meta; I guess we are designing for extensibility here? Why not just call this superconstructor `Transfer` (Transferable? WorkerTransferable?), and give it the |transfers| field directly? It doesn't seem likely to me that we'll need much more than that.

Comment on attachment 8526029 [details] [diff] [review] 2. Getting rid of OS.File toMsg/fromMsg pointer hackery, v2 Review of attachment 8526029 [details] [diff] [review]: ----------------------------------------------------------------- This overloading of outptr-ness doesn't seem right; the convention I thought we were following is: - in_ptr is stuff passed to the function to be computed on; - out_ptr is stuff that can be set by the function. But now we have a separate notion of outptr-ness, which is "something that is being passed to be computed on, but which the callee (silently, depending on the type) takes ownership of"--is my understanding correct here? That naming doesn't sound quite right. Also, could you please separate the actual pointer hack fixes from the various whitespace/naming/debugging fixes scattered throughout? Would make this a lot easier to review.

(In reply to Nathan Froyd (:froydnj) from comment #5) > Comment on attachment 8525980 [details] [diff] [review] > 1. New manner to specify transferables to a PromiseWorker > > Review of attachment 8525980 [details] [diff] [review]: > ----------------------------------------------------------------- > > So we're doing this so that individual objects that we send can flag > themselves as transferable without the sender needing to know anything about > transferability, correct? Yes. > Do we even need the explicit |transfers| argument anymore? Probably not, but I didn't want to spend time checking whether any client code was using it. > ::: toolkit/components/promiseworker/PromiseWorker.jsm > @@ +386,5 @@ > > + * @constructor > > + */ > > +BasePromiseWorker.Meta = function(data, meta) { > > + this.data = data; > > + this.meta = meta; > > I guess we are designing for extensibility here? Why not just call this > superconstructor `Transfer` (Transferable? WorkerTransferable?), and give > it the |transfers| field directly? It doesn't seem likely to me that we'll > need much more than that. It's for symmetry with the worker-side code, which uses Meta for both transfer and shutdown, but I can get rid of this.

(In reply to Nathan Froyd (:froydnj) from comment #6) > Comment on attachment 8526029 [details] [diff] [review] > 2. Getting rid of OS.File toMsg/fromMsg pointer hackery, v2 > > Review of attachment 8526029 [details] [diff] [review]: > ----------------------------------------------------------------- > > This overloading of outptr-ness doesn't seem right; the convention I thought > we were following is: > > - in_ptr is stuff passed to the function to be computed on; > - out_ptr is stuff that can be set by the function. > > But now we have a separate notion of outptr-ness, which is "something that > is being passed to be computed on, but which the callee (silently, depending > on the type) takes ownership of"--is my understanding correct here? That > naming doesn't sound quite right. You are right, something is very weird. This should actually be in_ptr stuff that is stolen. > Also, could you please separate the actual pointer hack fixes from the > various whitespace/naming/debugging fixes scattered throughout? Would make > this a lot easier to review. Er... I don't see any whitespace/naming/debugging fixes in my patch.

This issue is blocking us from landing the initial version of compacting GC. Yoric, do you know when you might have a fix for this? Let me know if there is anything I can do to help out.

I'll make this a Q1

I don't think I'll be able to rush this for the end of Q4. I will clearly make this a Q1 objective, with the aim of fixing this in January, unless some high-priority deadline prevents me from working on it. Is that sufficient for you?

(In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January 10th - use "needinfo") from comment #11) I don't have much time left in Q4 before I go on PTO, but ideally I'd like to land the current version of compacting in early January. Do you have a feel for how much work is left on this? If this is something I could take over myself I'm happy to do so.

(In reply to Nathan Froyd (:froydnj) from comment #6) > Comment on attachment 8526029 [details] [diff] [review] > 2. Getting rid of OS.File toMsg/fromMsg pointer hackery, v2 > > Review of attachment 8526029 [details] [diff] [review]: > ----------------------------------------------------------------- > > This overloading of outptr-ness doesn't seem right; the convention I thought > we were following is: > > - in_ptr is stuff passed to the function to be computed on; > - out_ptr is stuff that can be set by the function. > > But now we have a separate notion of outptr-ness, which is "something that > is being passed to be computed on, but which the callee (silently, depending > on the type) takes ownership of"--is my understanding correct here? That > naming doesn't sound quite right. /headwall For some reason, the code in osfile_async_front.jsm uses out_ptr wherever we should be using in_ptr, and vice-versa. If I just exchanged both, would that make it ok for you?

> Do you have a feel for how much work is left on this? If this is something I could take over myself I'm happy to do so. I don't think there is much work, but any error might be subtle to fix. I believe that patch 1 is good. For patch 2, the best course is probably to restart from scratch: - add a boolean argument `transfer` to methods `toMsg`/`fromMsg` of `in_ptr` and `out_ptr` (see my current patch 2 for how to implement it); - fix the uses of {in_ptr, out_ptr}.toMsg for OS.File.read, OS.File.prototype.read and OS.File.writeAtomic in osfile_async_front.jsm and osfile_async_worker.js to make use of the above argument. If you fell like doing it, go ahead. Otherwise, I'll do it in January.

This is what I've got so far. I've made toMsg() always transfer typed arrays and array buffers, because trying to convert them to a pointer will always throw an exception when bug 1080262 lands. (Really, it should convert for in params but complain for out params.) Since it made the internals simpler and because it doesn't seem to be used, I removed support for passing C pointers to the OS.File methods. Do you think that's going to be an acceptable change or should I put support for this back in?

Comment on attachment 8538526 [details] [diff] [review] bug1077354-patch-2-v2 work in progress Review of attachment 8538526 [details] [diff] [review]: ----------------------------------------------------------------- This looks good to me. ::: toolkit/components/osfile/modules/osfile_shared_allthreads.jsm @@ +458,5 @@ > return null; > } > if (typeof value == "string") { > return { string: value }; > } While this change should work, it assumes that add-ons are not using PtrType to transfer data. I think that this is the case, but could you check? ::: toolkit/components/osfile/modules/osfile_shared_front.jsm @@ +62,5 @@ > options = clone(maybeBytes); > maybeBytes = null; > } else { > options = clone(options || {}); > } Actually, we probably don't even need to clone `options`. @@ +63,5 @@ > maybeBytes = null; > } else { > options = clone(options || {}); > } > + let bytes = options.bytes; || undefined @@ +101,5 @@ > * > * @return {number} The number of bytes actually written. > */ > write: function write(buffer, options = {}) { > + let bytes = Nit: Whitespace.

(In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January 10th - use "needinfo") from comment #16) > This looks good to me. Great! > While this change should work, it assumes that add-ons are not using PtrType > to transfer data. I think that this is the case, but could you check? How can I check this?

Take a look at https://mxr.mozilla.org/addons/

(In reply to David Rajchenbach-Teller [:Yoric] (hard to reach until January 10th - use "needinfo") from comment #18) It seems that this will break FirefoxOS simulator addons, because these contain their own versions of the OS.File implementation. I think this is unavoidable however. I couldn't find anything else that this would conflict with.

I think that they ship an entire OS.File, so my guess is that this won't break.

Right, but it will break when we remove the CTypes feature that allows you to extract a pointer to the contents of an array buffer (bug 1080262).

BTW I had to make some changes to the first patch to get the linux B2G desktop mochitests to pass. I saw errors like this: 08:08:29 INFO - JavaScript error: resource://gre/modules/PromiseWorker.jsm, line 6: TypeError: this.BasePromiseWorker is undefined TBH I don't understand what's going on here but this fixed it.

Comment on attachment 8545210 [details] [diff] [review] bug1077354-patch-1-delta Review of attachment 8545210 [details] [diff] [review]: ----------------------------------------------------------------- Weird, but if it fixes the issue, I'm fine with that.

Patch to CTypes to allow DataView arguments to be passed as pointers.

Patch 1 with minor updates to fix test failures on B2G.

Patch to make OS.File transfer typed arrays and array buffers and remove support for passing C pointers. I checked and as far as I can tell this is not used.

Comment on attachment 8545290 [details] [diff] [review] bug1077354-patch-1 Review of attachment 8545290 [details] [diff] [review]: ----------------------------------------------------------------- Whoever commits this probably wants to clean up the commit message...

Comment on attachment 8545296 [details] [diff] [review] bug1077354-patch-2-v2 Review of attachment 8545296 [details] [diff] [review]: ----------------------------------------------------------------- Hooray for typed arrays everywhere.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b45b11002922 https://hg.mozilla.org/integration/mozilla-inbound/rev/e6ac6c211626 https://hg.mozilla.org/integration/mozilla-inbound/rev/b777166f71b0

https://hg.mozilla.org/mozilla-central/rev/b45b11002922 https://hg.mozilla.org/mozilla-central/rev/e6ac6c211626 https://hg.mozilla.org/mozilla-central/rev/b777166f71b0

I strongly suspect this has caused the test failure in bug 1119957. Here is a stripped-down version of the test that now fails: let test_appendToFile = function* () { const kStringToWrite = "Hello, world!"; let path = OS.Path.join(OS.Constants.Path.profileDir, "testFile.txt"); let textencoder = new TextEncoder(); let file = yield OS.File.open(path, {write: true}); let encodedString = textencoder.encode(kStringToWrite); dump(JSON.stringify(encodedString)+"\n"); yield file.write(encodedString); // Uncomment the following line to get the test to pass // encodedString = textencoder.encode(kStringToWrite); dump(JSON.stringify(encodedString)+"\n"); yield file.write(encodedString); yield file.close(); let text = (new TextDecoder()).decode(yield OS.File.read(path)); // The read text should be equal to kStringToWrite repeated twice. equal(text, kStringToWrite + kStringToWrite); yield OS.File.remove(path); } It is clear from the test output that encodedString is empty after the first OS.File.write call.

(In reply to aleth [:aleth] from comment #31) Yes, this change is to blame for the failure. OS.File methods that write array buffers will now neuter that array buffer. You will need to copy the buffer passed if you need to write it twice.

(In reply to Jon Coppeard (:jonco) from comment #32) > (In reply to aleth [:aleth] from comment #31) > Yes, this change is to blame for the failure. OS.File methods that write > array buffers will now neuter that array buffer. > > You will need to copy the buffer passed if you need to write it twice. Isn't this a bug? It's certainly unexpected that writing data deletes it ;) At the very least it seems this warrants a warning in the documentation, either for ArrayBuffers or for OS.File.

(In reply to aleth [:aleth] from comment #33) Yes I'll update the documentation to say this is what happens.

(In reply to Jon Coppeard (:jonco) from comment #34) > (In reply to aleth [:aleth] from comment #33) > Yes I'll update the documentation to say this is what happens. I think that updating the docs doesnt make everything ok. This has ruined a lot of stuff Ive been using OS.File heavily. Is it possible to make it not neuter it? Is there a reason to neutering it? I really appreciate all the work and dont want you to change for me, but i dont get why neutering it is ok?

If we do not neuter the buffer, by specification of Workers, the data is copied, which blocks the main thread during the copy. Since we regularly use OS.File to write buffers of several megabytes, this would be unacceptably slow. It's not that neutering the buffer is ok, it is that we didn't have any choice. The previous code was a hack, which depended on unspecified behavior from the garbage-collector, and which could crash the browser or corrupt its memory badly. The JS team has recently changed the garbage-collecting strategy, which made the previous hack unusable.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #36) > If we do not neuter the buffer, by specification of Workers, the data is > copied, which blocks the main thread during the copy. Since we regularly use > OS.File to write buffers of several megabytes, this would be unacceptably > slow. > > It's not that neutering the buffer is ok, it is that we didn't have any > choice. The previous code was a hack, which depended on unspecified behavior > from the garbage-collector, and which could crash the browser or corrupt its > memory badly. The JS team has recently changed the garbage-collecting > strategy, which made the previous hack unusable. Oh thanks!! This makes sense, I guess I don't really need to deal with my array buffer after writing it to disk. I can just do it before. I was saving some icons then using the arraybuffer for some stuff like HICON later on but i can do that first. or if i really need ot then i can copy it. thanks for the explanation! performance increase is definitely cool! i dont understand it it totally but the way you put it sounds real good!