Closed Bug 795687 Opened 8 years ago Closed 8 years ago

[OS.File] Use typed arrays instead of ArrayBuffer

Categories

(Toolkit :: OS.File, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 5 obsolete files)

For the moment, |read| and |readAll| return |{buffer: ArrayBuffer, bytes:number}|. 
Before this reaches Aurora, I would like to replace this API with a Uint8Array, which seems to be much more idiomatic (see e.g. bug 764234).
Here we go. Attaching a patch that I hope we can merge into 18, so as to avoid breaking the API once the library has reached Aurora.
Assignee: nobody → dteller
Attachment #666299 - Flags: review?(nfroyd)
Going further, we can actually rather easily refactor OS.File to accept typed arrays instead of ArrayBuffer. I believe that this makes the API much nicer and less error-prone, so I would like your feedback on that.
Attachment #666515 - Flags: feedback?(nfroyd)
Comment on attachment 666515 [details] [diff] [review]
2. Refactoring the rest of OS.File to accept typed arrays

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

I think writeAtomic, at least, needs some kind of fixup too.  Seems like there ought to be more documentation fixes too.

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +317,5 @@
>     */
>    readTo: function readTo(buffer, options) {
> +    // If |buffer| is an ArrayBuffer or typed array and there is no
> +    // |bytes| options, we need to extract the |byteLength| now, as it
> +    // will be lost by communication

First off, the documentation says that we don't take ArrayBuffers any more.

Secondly, if you're not going to specify what kind of typed buffer you need, you should be using BYTES_PER_ELEMENT to figure out how many bytes you're going to read.  (I do think it would be potentially valuable to be able to pass any kind of typed array here, not just uint8 ones, but there are opportunities for footguns.)
Attachment #666515 - Flags: feedback?(nfroyd) → feedback+
Comment on attachment 666299 [details] [diff] [review]
Refactoring OS.File.read/OS.File.prototype.read to return a Uint8Array

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

A lot of the tests look like they have been incompletely rewritten; I see a lot of .length and .buffer accesses on the same object, which can't be right.

The two patches in this bug thus far should be committed as a single patch, even if they're broken into separate patches for review.

::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
@@ +476,4 @@
>      promise = promise.then(
>        function read_complete(contents) {
>          test.ok(contents, "Obtained contents");
>          buffer = contents.buffer;

I don't think .buffer can be correct here.

@@ -489,5 @@
>  // Check that options are not altered
>  
>      promise = promise.then(
>        function atomicWrite_complete(bytesWritten) {
> -        test.is(bytes, bytesWritten, "Wrote the correct number of bytes");

We should still check this.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +341,3 @@
>   
>    // File.writeAtomic on top of nothing
>    OS.File.writeAtomic(tmp_file_name, readResult.buffer,

.buffer can't be right.

@@ +369,5 @@
>    dest = OS.File.open(tmp_file_name, {write: true, trunc:true});
>    dest.setPosition(1234);
>    dest.close();
>  
>    OS.File.writeAtomic(tmp_file_name, readResult.buffer,

Here too.
Attachment #666299 - Flags: review?(nfroyd) → review-
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Comment on attachment 666299 [details] [diff] [review]
> Refactoring OS.File.read/OS.File.prototype.read to return a Uint8Array
> 
> Review of attachment 666299 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> A lot of the tests look like they have been incompletely rewritten; I see a
> lot of .length and .buffer accesses on the same object, which can't be right.

Actually, all typed arrays have:
- .length (number of items, as regular arrays);
- .buffer (the underlying buffer);
- .byteLength;
- .byteOffset.

So nothing wrong here.

> The two patches in this bug thus far should be committed as a single patch,
> even if they're broken into separate patches for review.
> 
> ::: toolkit/components/osfile/tests/mochi/main_test_osfile_async.js
> @@ +476,4 @@
> >      promise = promise.then(
> >        function read_complete(contents) {
> >          test.ok(contents, "Obtained contents");
> >          buffer = contents.buffer;
> 
> I don't think .buffer can be correct here.

I am pretty sure that it is.

> @@ -489,5 @@
> >  // Check that options are not altered
> >  
> >      promise = promise.then(
> >        function atomicWrite_complete(bytesWritten) {
> > -        test.is(bytes, bytesWritten, "Wrote the correct number of bytes");
> 
> We should still check this.

ok

> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
> @@ +341,3 @@
> >   
> >    // File.writeAtomic on top of nothing
> >    OS.File.writeAtomic(tmp_file_name, readResult.buffer,
> 
> .buffer can't be right.

It is right, although if we fold both patches, it becomes unnecessary.

> 
> @@ +369,5 @@
> >    dest = OS.File.open(tmp_file_name, {write: true, trunc:true});
> >    dest.setPosition(1234);
> >    dest.close();
> >  
> >    OS.File.writeAtomic(tmp_file_name, readResult.buffer,
> 
> Here too.

Here too :)(In reply to Nathan Froyd (:froydnj) from comment #3)
> Comment on attachment 666515 [details] [diff] [review]
> 2. Refactoring the rest of OS.File to accept typed arrays
> 
> Review of attachment 666515 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think writeAtomic, at least, needs some kind of fixup too.  Seems like
> there ought to be more documentation fixes too.

You are quite right.

> 
> ::: toolkit/components/osfile/osfile_async_front.jsm
> @@ +317,5 @@
> >     */
> >    readTo: function readTo(buffer, options) {
> > +    // If |buffer| is an ArrayBuffer or typed array and there is no
> > +    // |bytes| options, we need to extract the |byteLength| now, as it
> > +    // will be lost by communication
> 
> First off, the documentation says that we don't take ArrayBuffers any more.

Fair enough.

> Secondly, if you're not going to specify what kind of typed buffer you need,
> you should be using BYTES_PER_ELEMENT to figure out how many bytes you're
> going to read.  (I do think it would be potentially valuable to be able to
> pass any kind of typed array here, not just uint8 ones, but there are
> opportunities for footguns.)

I am not sure why we would need that. Since everything is specified in bytes anyway (either with option |bytes| or with |byteLength|), how would this would be useful?
(In reply to David Rajchenbach Teller [:Yoric] from comment #5)
> > A lot of the tests look like they have been incompletely rewritten; I see a
> > lot of .length and .buffer accesses on the same object, which can't be right.
> 
> Actually, all typed arrays have:
> - .length (number of items, as regular arrays);
> - .buffer (the underlying buffer);
> - .byteLength;
> - .byteOffset.
> 
> So nothing wrong here.

Doh, I missed the |implements ArrayBufferView| in the typed arrays spec.  Yes, mea culpa.

> > Secondly, if you're not going to specify what kind of typed buffer you need,
> > you should be using BYTES_PER_ELEMENT to figure out how many bytes you're
> > going to read.  (I do think it would be potentially valuable to be able to
> > pass any kind of typed array here, not just uint8 ones, but there are
> > opportunities for footguns.)
> 
> I am not sure why we would need that. Since everything is specified in bytes
> anyway (either with option |bytes| or with |byteLength|), how would this
> would be useful?

Never mind me!  Bad idea to do reviews in the morning, it looks like.
Here we go. Applied feedback, folded patches, and updated a few tests even further.
Attachment #666299 - Attachment is obsolete: true
Attachment #666515 - Attachment is obsolete: true
Attachment #666564 - Flags: review?(nfroyd)
Summary: [OS.File] read should return a Uint8Array → [OS.File] Use typed arrays instead of ArrayBuffer
Duplicate of this bug: 795565
Comment on attachment 666564 [details] [diff] [review]
Refactoring OS.File to use typed arrays everywhere

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

Comments below.  I didn't look terribly closely at the tests, but everything looks more-or-less OK.

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +281,5 @@
> +           normalized = Types.uint8_t.in_ptr.implementation(value.buffer);
> +           if (value.byteOffset != 0) {
> +             normalized = exports.OS.Shared.offsetBy(normalized, value.byteOffset);
> +           }
> +         } else { // ArrayBuffer

Do we still need this case?  The documentation states that we don't take ArrayBuffers anymore, but we still have various internal functions that would silently accept ArrayBuffers where we would take typed arrays.  If we don't need this case, it'd be better to get rid of it.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ -74,5 @@
>     * accomodate |bytes| bytes.
>     * @param {*=} options Optionally, an object that may contain the
>     * following fields:
> -   * - {number} offset The offset in |buffer| at which to start placing
> -   * data

I still think this is useful.  It means that users don't have to do pointer arithmetic and/or construct separate views into their typed array before passing it to this function.

I guess you still have to construct a separate object for the options dictionary, so whether passing offset is more efficient than constructing a separate array is dependent upon the vagaries of the JS engine.

@@ +151,5 @@
>      if (bytes == null) {
>        throw new TypeError("C pointer missing bytes indication.");
>      }
>    } else if ("byteLength" in candidate) {
> +    // ArrayBuffer or Typed Array

We should disallow ArrayBuffers if possible.

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +79,5 @@
>  
>       /**
>        * Read some bytes from a file.
>        *
> +      * @param {Typed array} buffer A buffer for holding the data

Is this really correct?  It looks like normalizeToPointer always makes sure you're going to get a C pointer here.
Attachment #666564 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #9)
> Comment on attachment 666564 [details] [diff] [review]
> Refactoring OS.File to use typed arrays everywhere
> 
> Review of attachment 666564 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Comments below.  I didn't look terribly closely at the tests, but everything
> looks more-or-less OK.
> 
> ::: toolkit/components/osfile/osfile_shared_allthreads.jsm
> @@ +281,5 @@
> > +           normalized = Types.uint8_t.in_ptr.implementation(value.buffer);
> > +           if (value.byteOffset != 0) {
> > +             normalized = exports.OS.Shared.offsetBy(normalized, value.byteOffset);
> > +           }
> > +         } else { // ArrayBuffer
> 
> Do we still need this case?  The documentation states that we don't take
> ArrayBuffers anymore, but we still have various internal functions that
> would silently accept ArrayBuffers where we would take typed arrays.  If we
> don't need this case, it'd be better to get rid of it.

Ok. My reflex was "since it's basically for free, let's accept it", but I can remove it.

> 
> ::: toolkit/components/osfile/osfile_shared_front.jsm
> @@ -74,5 @@
> >     * accomodate |bytes| bytes.
> >     * @param {*=} options Optionally, an object that may contain the
> >     * following fields:
> > -   * - {number} offset The offset in |buffer| at which to start placing
> > -   * data
> 
> I still think this is useful.  It means that users don't have to do pointer
> arithmetic and/or construct separate views into their typed array before
> passing it to this function.
> 
> I guess you still have to construct a separate object for the options
> dictionary, so whether passing offset is more efficient than constructing a
> separate array is dependent upon the vagaries of the JS engine.

I have the feeling that this is actually not useful. Users can have the exact same feature by constructing views, and this seems to be the recommended manner of working with raw data (see e.g. http://wiki.whatwg.org/wiki/StringEncoding#TextDecoder).

Let me suggest the following: land without the feature, add it later if users start to request it.

> 
> @@ +151,5 @@
> >      if (bytes == null) {
> >        throw new TypeError("C pointer missing bytes indication.");
> >      }
> >    } else if ("byteLength" in candidate) {
> > +    // ArrayBuffer or Typed Array
> 
> We should disallow ArrayBuffers if possible.

ok
 
> ::: toolkit/components/osfile/osfile_unix_front.jsm
> @@ +79,5 @@
> >  
> >       /**
> >        * Read some bytes from a file.
> >        *
> > +      * @param {Typed array} buffer A buffer for holding the data
> 
> Is this really correct?  It looks like normalizeToPointer always makes sure
> you're going to get a C pointer here.

Good point.
Attachment #666564 - Attachment is obsolete: true
Attachment #666774 - Flags: review?(nfroyd)
(In reply to David Rajchenbach Teller [:Yoric] from comment #10)
> > ::: toolkit/components/osfile/osfile_shared_allthreads.jsm
> > @@ +281,5 @@
> > > +           normalized = Types.uint8_t.in_ptr.implementation(value.buffer);
> > > +           if (value.byteOffset != 0) {
> > > +             normalized = exports.OS.Shared.offsetBy(normalized, value.byteOffset);
> > > +           }
> > > +         } else { // ArrayBuffer
> > 
> > Do we still need this case?  The documentation states that we don't take
> > ArrayBuffers anymore, but we still have various internal functions that
> > would silently accept ArrayBuffers where we would take typed arrays.  If we
> > don't need this case, it'd be better to get rid of it.
> 
> Ok. My reflex was "since it's basically for free, let's accept it", but I
> can remove it.

I think it's better to be stricter in what we accept here.  And if it turns out that it would be more convenient, we only have to update the code in one or two places thanks to all the refactoring we did before the code was committed. ;)

> > I still think this is useful.  It means that users don't have to do pointer
> > arithmetic and/or construct separate views into their typed array before
> > passing it to this function.
> 
> I have the feeling that this is actually not useful. Users can have the
> exact same feature by constructing views, and this seems to be the
> recommended manner of working with raw data (see e.g.
> http://wiki.whatwg.org/wiki/StringEncoding#TextDecoder).
> 
> Let me suggest the following: land without the feature, add it later if
> users start to request it.

OK.  I guess if views are cheap enough, the distinction probably doesn't matter.  And if the standard library is doing things one way, we should try to follow it.
Comment on attachment 666774 [details] [diff] [review]
Refactoring OS.File to use typed arrays everywhere, v2

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

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +318,5 @@
>    readTo: function readTo(buffer, options) {
> +    // If |buffer| is a typed array and there is no |bytes| options, we
> +    // need to extract the |byteLength| now, as it will be lost by
> +    // communication
> +    if ("byteLength" in buffer && (!options || !"bytes" in options)) {

Since we're disallowing ArrayBuffers, we should probably find some other test for typed array-ness than |"byteLength" in buffer|, given that both typed arrays and ArrayBuffers implement this property.  Can we use instanceof here, or maybe BYTES_PER_ELEMENT?  Isee you use byteOffset && byteLength elsewhere; maybe that is sufficient.  (And maybe that should be moved into a helper function? :)

Lots of places will need to be updated with this change.

@@ +397,4 @@
>      promise = promise.then(
> +      function withSize(aSize) {
> +        size = aSize;
> +        array = new Uint8Array(ArrayBuffer(size));

If I'm reading the spec correctly, you should be able to do |new Uint8Array(size)| directly.  Here and elsewhere.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +127,5 @@
>    }
>  };
>  
>  /**
> + * Utility function used to normalize a Typed Array, ArrayBuffer or C

Still with the ArrayBuffers in the documentation.

@@ +145,2 @@
>    if (!candidate) {
> +    throw new TypeError("Expecting a C pointer, an ArrayBuffer or a Typed Array");

We no longer accept ArrayBuffers.  Please fix all relevant error messages.
Attachment #666774 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #13)
> Comment on attachment 666774 [details] [diff] [review]
> Refactoring OS.File to use typed arrays everywhere, v2
> 
> Review of attachment 666774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_async_front.jsm
> @@ +318,5 @@
> >    readTo: function readTo(buffer, options) {
> > +    // If |buffer| is a typed array and there is no |bytes| options, we
> > +    // need to extract the |byteLength| now, as it will be lost by
> > +    // communication
> > +    if ("byteLength" in buffer && (!options || !"bytes" in options)) {
> 
> Since we're disallowing ArrayBuffers, we should probably find some other
> test for typed array-ness than |"byteLength" in buffer|, given that both
> typed arrays and ArrayBuffers implement this property.  Can we use
> instanceof here, or maybe BYTES_PER_ELEMENT?  Isee you use byteOffset &&
> byteLength elsewhere; maybe that is sufficient.  (And maybe that should be
> moved into a helper function? :)

Well, we cannot use BYTES_PER_ELEMENT as it does not work with DataView. I will put the byteOffset && byteLength test as a utility function.

> 
> Lots of places will need to be updated with this change.
> 
> @@ +397,4 @@
> >      promise = promise.then(
> > +      function withSize(aSize) {
> > +        size = aSize;
> > +        array = new Uint8Array(ArrayBuffer(size));
> 
> If I'm reading the spec correctly, you should be able to do |new
> Uint8Array(size)| directly.  Here and elsewhere.

ok

> ::: toolkit/components/osfile/osfile_shared_front.jsm
> @@ +127,5 @@
> >    }
> >  };
> >  
> >  /**
> > + * Utility function used to normalize a Typed Array, ArrayBuffer or C
> 
> Still with the ArrayBuffers in the documentation.

ok

> @@ +145,2 @@
> >    if (!candidate) {
> > +    throw new TypeError("Expecting a C pointer, an ArrayBuffer or a Typed Array");
> 
> We no longer accept ArrayBuffers.  Please fix all relevant error messages.

done
Attachment #666774 - Attachment is obsolete: true
Attachment #666814 - Flags: review?(nfroyd)
Comment on attachment 666814 [details] [diff] [review]
Refactoring OS.File to use typed arrays everywhere, v3

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

::: toolkit/components/osfile/osfile_async_front.jsm
@@ +331,3 @@
>      return Scheduler.post("File_prototype_readTo",
>        [this._fdmsg,
> +       Type.void_t.out_ptr.toMsg(buffer, options),

It looks like toMsg was not suitably modified to accept options in this patch.

::: toolkit/components/osfile/osfile_shared_front.jsm
@@ +163,2 @@
>      }
> +    if (candidate.byteLength < bytes) {

Nit: I think I would make this the else branch of the above conditional; no point in checking this condition if bytes was originally null and it's a tad clearer.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ -309,5 @@
> -
> -  readResult = source.readTo(ptr, {bytes: LEFT, offset: OFFSET});
> -  is(readResult, LEFT, "test_readall_writeall_file: read the right number of bytes (with offset)");
> -
> -  dest.write(ptr, {bytes: LEFT, offset: OFFSET});

Is there a reason why this block of test disappeared?
Attachment #666814 - Flags: review?(nfroyd) → review-
(In reply to Nathan Froyd (:froydnj) from comment #16)
> Comment on attachment 666814 [details] [diff] [review]
> Refactoring OS.File to use typed arrays everywhere, v3
> 
> Review of attachment 666814 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_async_front.jsm
> @@ +331,3 @@
> >      return Scheduler.post("File_prototype_readTo",
> >        [this._fdmsg,
> > +       Type.void_t.out_ptr.toMsg(buffer, options),
> 
> It looks like toMsg was not suitably modified to accept options in this
> patch.

Actually, it turns out that |options| is not required, and I should have reverted this line to its previous state.

> 
> ::: toolkit/components/osfile/osfile_shared_front.jsm
> @@ +163,2 @@
> >      }
> > +    if (candidate.byteLength < bytes) {
> 
> Nit: I think I would make this the else branch of the above conditional; no
> point in checking this condition if bytes was originally null and it's a tad
> clearer.

ok

> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
> @@ -309,5 @@
> > -
> > -  readResult = source.readTo(ptr, {bytes: LEFT, offset: OFFSET});
> > -  is(readResult, LEFT, "test_readall_writeall_file: read the right number of bytes (with offset)");
> > -
> > -  dest.write(ptr, {bytes: LEFT, offset: OFFSET});
> 
> Is there a reason why this block of test disappeared?

Well, the reason is that option |offset| has disappeared, so the test did not really make sense anymore.
Feedback applied.
Attachment #666814 - Attachment is obsolete: true
Attachment #667000 - Flags: review?(nfroyd)
Comment on attachment 667000 [details] [diff] [review]
Refactoring OS.File to use typed arrays everywhere, v4

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

We have converged!  Thanks for your patience.
Attachment #667000 - Flags: review?(nfroyd) → review+
https://hg.mozilla.org/mozilla-central/rev/531b6090f45e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.