Closed Bug 771928 Opened 12 years ago Closed 12 years ago

[OS.File] Convert ArrayBuffer to C pointer + pointer arithmetics

Categories

(Toolkit Graveyard :: OS.File, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla17

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 13 obsolete files)

7.04 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
2.92 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
We need to implement ArrayBuffer to C pointer conversion.

To implement |readAll|, we will also certainly want pointer arithmetics.

As it turns out, both features can be implemented with what is essentially a one-liner in C, exported through osfileutils.cpp.
Attached patch Native code (obsolete) — Splinter Review
Assignee: nobody → dteller
Attachment #644659 - Flags: review?(nfroyd)
Attached patch JS bindings (obsolete) — Splinter Review
Attachment #644660 - Flags: review?(nfroyd)
Attachment #644660 - Attachment is patch: true
Attached patch Companion testsuite (obsolete) — Splinter Review
Here we go. Nfroyd, I hope you are ready for some serious JavaScript pointer arithmetics :)

There are essentially two reasons for which we need this:
- to be able to use OS.File methods read/write at an offset of an array/pointer;
- to serialize occurrences of ArrayBuffer without stealing them from their owning thread (see dependent bug).
Attachment #644661 - Flags: review?(nfroyd)
Comment on attachment 644661 [details] [diff] [review]
Companion testsuite

What's the functional difference between the offsetBy walking and the pointer arithmetic walking? Does they actually exercise different code?
(In reply to Josh Matthews [:jdm] from comment #4)
> Comment on attachment 644661 [details] [diff] [review]
> Companion testsuite
> 
> What's the functional difference between the offsetBy walking and the
> pointer arithmetic walking? Does they actually exercise different code?

No, it's just the name of the test that does not really match its contents. What we are basically doing there is that |offsetBy(foo, 0)| returns |foo|. I could just have compared the values, of course.
I don't understand the rationale behind this set of patches.  We can already do address manipulation on ArrayBuffers in pure JS.  IIUC, we have js-ctypes code to coerce ArrayBuffers to C pointers (which seems to address--pun intended--the serialization of ArrayBuffer use case).  We have code to get at raw C pointers in js-ctypes (addressOfElement for arrays, addressOfField for struct types) and code to manipulate them (.increment()/.decrement()).  What is this patch set buying us that some combination of those features do not?
(In reply to Nathan Froyd (:froydnj) from comment #6)
> I don't understand the rationale behind this set of patches.  We can already
> do address manipulation on ArrayBuffers in pure JS. IIUC, we have js-ctypes
> code to coerce ArrayBuffers to C pointers

Yes, I wrote that code :)

> (which seems to address--pun
> intended--the serialization of ArrayBuffer use case).
>  We have code to get
> at raw C pointers in js-ctypes (addressOfElement for arrays, addressOfField
> for struct types) and code to manipulate them (.increment()/.decrement()). 
> What is this patch set buying us that some combination of those features do
> not?

You are right that we can get a pointer to an ArrayBuffer with a judicious use of js-ctypes, without offsetBy. Indeed, this removes one of the usage scenarios of this patch.

Now, we might be able to get rid of the second scenario: that of advancing of, say, a pointer |buf| of 1024 bytes (I only care about bytes for my code). We can try a few manners of handling this scenario without offsetBy, but none to my satisfaction:

Technique #1: increment
let addr = ctypes.cast(buf, ctypes.uint8_t.Ptr);
for (let i = 0; i < 1024; ++i) addr = addrincrement();
return addr;

(linear time, linear space, clumsy)


Technique #2: cast to array
let addr = ctypes.cast(buf, ctypes.uint8_t.array(1024).ptr);
return addr.content.addressOfElement(1024);

(still rather clumsy, requires creating a new type each time, but perhaps acceptable)


Technique #3
let fake_type = ctypes.StructType("fake_type", [{padding: ctypes.uint8_t.array(1024)}, {target:uint8_t}]);
let addr = ctypes.cast(buf, fake_type.ptr);
return addr.contents.addressOfField("target");

(clumsier and slower)

---

Now, if you think that technique #2 is acceptable, I'll mark this bug as WONTFIX and use that technique.
(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #7)
> Now, we might be able to get rid of the second scenario: that of advancing
> of, say, a pointer |buf| of 1024 bytes (I only care about bytes for my
> code). We can try a few manners of handling this scenario without offsetBy,
> but none to my satisfaction:

Yes, none of those are very nice.  I see that js-ctypes's increment() method doesn't permit you to specify the offset; there was some discussion about arbitrary offsets in bug 696450, but that functionality was deferred.  I think the time has come for that functionality.

That means we would have technique #4:

  let addr = ctypes.cast(buf, ctypes.uint8_t.ptr);
  return addr.increment(1024);

constant time, constant (?) space (presumably because we're not creating a new type each time).  Also avoids footguns with trying to figure out the proper byte offset for non-byte pointers.

It may be appropriate to still have Pointers.offsetBy that does the right thing for CData or ArrayBuffers to make client code nicer.  Something like:

  offsetBy(pointer, amount) -> return pointer.increment(amount)
  offsetBy(arrayBuffer, amount, [elementType]) -> technique #4
    if elementType is not provided, assume arrayBuffer is interpreted as uint8_t*.
    otherwise, assume arrayBuffer is interpreted as elementType*.

WDYT?
Guess it's worth noting that arbitrary pointer arithmetic is a big gun to hand to people, and bholley was skeptical about adding it in bug 696450, instead proposing technique #2 as a workaround.
Ok, let's WONTFIX this for the moment.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Attachment #644659 - Flags: review?(nfroyd)
Attachment #644660 - Flags: review?(nfroyd)
Attachment #644661 - Flags: review?(nfroyd)
Attached patch offsetBy, v2 (obsolete) — Splinter Review
I am reopening this bug as I now need to advance pointers by arbitrary amounts (to implement readAll), so here is a new implementation of |offsetBy| without going through dynamically linked code, as discussed above.
Attachment #644659 - Attachment is obsolete: true
Attachment #644660 - Attachment is obsolete: true
Attachment #646608 - Flags: review?(nfroyd)
Attached patch Companion testsuite (obsolete) — Splinter Review
Attachment #644661 - Attachment is obsolete: true
Attachment #646609 - Flags: review?(nfroyd)
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Comment on attachment 646608 [details] [diff] [review]
offsetBy, v2

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

You seem to have attached the test suite patch twice?
Attachment #646608 - Flags: review?(nfroyd)
Comment on attachment 646609 [details] [diff] [review]
Companion testsuite

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

Just f+'ing at this point because I have questions, but the substance looks good.

Having not seen the actual implementation yet: does offsetBy work on typed arrays or just ArrayBuffers?  If so, a test or two in that direction would be good.  If not...why not? :)

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +290,5 @@
> +function test_offsetby() {
> +  ok(true, "Starting test_offsetby");
> +
> +  let offsetBy = OS.Shared.Utils.Pointers.offsetBy;
> +  isnot(!!offsetBy, false, "test_offsetby: Definition of offsetBy");

Holy quadruple negative, Batman!

Maybe:

is(!!offsetBy, true, ...);

or better:

is(typeof offsetBy, "function", ...);

@@ +296,5 @@
> +  // Initialize one array
> +  let LENGTH = 1024;
> +  let buf = new ArrayBuffer(LENGTH);
> +  let view = new Uint8Array(buf);
> +  let i;

Nit: please just 'let i = 0' in all the for loops and remove this declaration.

@@ +304,5 @@
> +
> +  // Walk through the array with offsetBy
> +  for (i = 0; i < LENGTH; ++i) {
> +    let voidptr = offsetBy(buf, i);
> +    let intptr = OS.Shared.Type.uint8_t.in_ptr.cast(voidptr);

This cast looks like it would get tedious.  Is there any reason not to have offsetBy(X, I) return a uint8_t pointer?

@@ +314,5 @@
> +
> +  // Ensure that offsetBy(..., 0) is idempotent
> +  let startptr = offsetBy(buf, 0);
> +  let startptr2 = offsetBy(startptr, 0);
> +  is("" + startptr, "" + startptr2, "test_offsetby: offsetBy(..., 0) is idempotent");

Nit: is(startptr.toString(), startptr2.toString(), ...); looks better to me.

@@ +319,5 @@
> +
> +  let view2 = new Uint16Array(buf);
> +  for (i = 0; i < LENGTH/2; ++i) {
> +    let voidptr = offsetBy(buf, i, ctypes.uint16_t);
> +    let int16ptr = OS.Shared.Type.uint16_t.in_ptr.cast(voidptr);

I would expect this cast to be unnecessary given the explicit uint16_t element type provided to offsetBy.  Please add the necessary magic to offsetBy to make this go away.

Oh, hm, we pass ctypes.uint16_t here and cast to Type.uint16_t.  Is there a reason you don't just pass Type.uint16_t?
Attachment #646609 - Flags: review?(nfroyd) → feedback+
Attached patch offsetBy, v2 (obsolete) — Splinter Review
Ah, sorry.
Attachment #646608 - Attachment is obsolete: true
Attachment #646976 - Flags: review?(nfroyd)
(In reply to Nathan Froyd (:froydnj) from comment #14)
> Comment on attachment 646609 [details] [diff] [review]
> Companion testsuite
> 
> Review of attachment 646609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just f+'ing at this point because I have questions, but the substance looks
> good.
> 
> Having not seen the actual implementation yet: does offsetBy work on typed
> arrays or just ArrayBuffers?  If so, a test or two in that direction would
> be good.  If not...why not? :)

Only ArrayBuffer, atm, because doing anything else would require a patch to js-ctypes.
I might provide that patch at some point, but I am trying to keep this out of the critical path.

> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
> @@ +290,5 @@
> > +function test_offsetby() {
> > +  ok(true, "Starting test_offsetby");
> > +
> > +  let offsetBy = OS.Shared.Utils.Pointers.offsetBy;
> > +  isnot(!!offsetBy, false, "test_offsetby: Definition of offsetBy");
> 
> Holy quadruple negative, Batman!
> 
> Maybe:
> 
> is(!!offsetBy, true, ...);
> 
> or better:
> 
> is(typeof offsetBy, "function", ...);

Ok, ok :)

> @@ +296,5 @@
> > +  // Initialize one array
> > +  let LENGTH = 1024;
> > +  let buf = new ArrayBuffer(LENGTH);
> > +  let view = new Uint8Array(buf);
> > +  let i;
> 
> Nit: please just 'let i = 0' in all the for loops and remove this
> declaration.
> 
> @@ +304,5 @@
> > +
> > +  // Walk through the array with offsetBy
> > +  for (i = 0; i < LENGTH; ++i) {
> > +    let voidptr = offsetBy(buf, i);
> > +    let intptr = OS.Shared.Type.uint8_t.in_ptr.cast(voidptr);
> 
> This cast looks like it would get tedious.  Is there any reason not to have
> offsetBy(X, I) return a uint8_t pointer?

What is the idea? Would you want offsetBy to be return a type parameterized by the type of its argument? I can do that, but this will require a js-ctypes patch to get this to work with a CDataFinalizer-wrapped pointer: for the moment, there is no good way to extract the type of a CDataFinalizer-based value. The data is available in the native code, though, so I could make it public.

Alternatively, I can automatically cast to the optional type argument if it is provided. This does not require any patch to js-ctypes.

> 
> @@ +314,5 @@
> > +
> > +  // Ensure that offsetBy(..., 0) is idempotent
> > +  let startptr = offsetBy(buf, 0);
> > +  let startptr2 = offsetBy(startptr, 0);
> > +  is("" + startptr, "" + startptr2, "test_offsetby: offsetBy(..., 0) is idempotent");
> 
> Nit: is(startptr.toString(), startptr2.toString(), ...); looks better to me.

As you prefer. I took the habit of |"" + ...| to avoid errors with |null|/|undefined|.


> 
> @@ +319,5 @@
> > +
> > +  let view2 = new Uint16Array(buf);
> > +  for (i = 0; i < LENGTH/2; ++i) {
> > +    let voidptr = offsetBy(buf, i, ctypes.uint16_t);
> > +    let int16ptr = OS.Shared.Type.uint16_t.in_ptr.cast(voidptr);
> 
> I would expect this cast to be unnecessary given the explicit uint16_t
> element type provided to offsetBy.  Please add the necessary magic to
> offsetBy to make this go away.

I can do that in most cases, but for CDataFinalizer-based pointers, I have the same issue as above, and this would require the same patch to js-ctypes.

> Oh, hm, we pass ctypes.uint16_t here and cast to Type.uint16_t.  Is there a
> reason you don't just pass Type.uint16_t?

I could pass Type.uint_t instead of ctypes.uint16_t without any difference. For the casting, see above.
Attaching a variant (untested so far) that casts automatically to the optional type argument if it is provided.
Attachment #646979 - Flags: feedback?(nfroyd)
Component: Networking: File → OS.File
Product: Core → Toolkit
Nathan, what would you think of the following variant?

OS.Shared.Utils.Pointers.offsetBy(ptr or ArrayBuffer, bytes)
- takes a pointer (of any type), increment it by |bytes| bytes, return a |void*|

OS.Shared.PtrType.prototype.offsetBy(ptr or ArrayBuffer, index)
- takes a pointer (of any type), increment it by |index * this.size| bytes, return a pointer of |this| type;
- special case for |void*|, which replaces |this.size| with 1.

The main role of the first function being to implement the method.
(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #16)
> (In reply to Nathan Froyd (:froydnj) from comment #14)
> > Comment on attachment 646609 [details] [diff] [review]
> > Having not seen the actual implementation yet: does offsetBy work on typed
> > arrays or just ArrayBuffers?  If so, a test or two in that direction would
> > be good.  If not...why not? :)
> 
> Only ArrayBuffer, atm, because doing anything else would require a patch to
> js-ctypes.
> I might provide that patch at some point, but I am trying to keep this out
> of the critical path.

Fair enough.  Now that I think about it, offsetBy(uint64_array, amount, Type.uint16_t) is liable to be confusing. :)

> > > +    let intptr = OS.Shared.Type.uint8_t.in_ptr.cast(voidptr);
> > 
> > This cast looks like it would get tedious.  Is there any reason not to have
> > offsetBy(X, I) return a uint8_t pointer?
> 
> What is the idea? Would you want offsetBy to be return a type parameterized
> by the type of its argument? I can do that, but this will require a
> js-ctypes patch to get this to work with a CDataFinalizer-wrapped pointer:
> for the moment, there is no good way to extract the type of a
> CDataFinalizer-based value. The data is available in the native code,
> though, so I could make it public.
> 
> Alternatively, I can automatically cast to the optional type argument if it
> is provided. This does not require any patch to js-ctypes.

The latter would be a nice extension (and provided by subsequent patches), but the first is really what I was getting at in this case.  It seems strange to offset a (typed) pointer and get back a void*.

Although I guess in this case you're offsetBy'ing an ArrayBuffer, so there's no typed pointer involved.  Hm.  In this case, it'd be handy to have offsetBy(arrayBuffer, offset) return a uint8_t*, but it's not obvious that you always want that.

So, let's say:

- We want the optional type argument to define the return type of the pointer.  (Done with your patches here.)
- We want the returned pointer to be parameterized by the type of the argument pointer.  Please file a followup bug for that; your choice whether it blocks this one or not.
- Keep offsetBy(arrayBuffer, offset) returning void* and we'll see how (some) client code looks like with that.  If it gets tedious, we may revisit this choice.  File a bug on this if you like so we don't lose track of it.

Sound good?

> > > +  is("" + startptr, "" + startptr2, "test_offsetby: offsetBy(..., 0) is idempotent");
> > 
> > Nit: is(startptr.toString(), startptr2.toString(), ...); looks better to me.
> 
> As you prefer. I took the habit of |"" + ...| to avoid errors with
> |null|/|undefined|.

Ah, my JS-fu is weak.  In that case, I think the original is fine.

> > Oh, hm, we pass ctypes.uint16_t here and cast to Type.uint16_t.  Is there a
> > reason you don't just pass Type.uint16_t?
> 
> I could pass Type.uint_t instead of ctypes.uint16_t without any difference.
> For the casting, see above.

Given the stated purpose of Type is to insulate people from ctypes whenever possible, I think it preferable to use Type in other OS.File code.  People will poke at the implementation for guidance on how to write their own stuff and will get confused if some places use ctypes.foo and other places use Type.foo.  (Like me!)  If ctypes has to be used outside of Type itself, there should be explanatory comments saying why.
(In reply to David Rajchenbach Teller (away until early August) [:Yoric] from comment #18)
> Nathan, what would you think of the following variant?
> 
> OS.Shared.Utils.Pointers.offsetBy(ptr or ArrayBuffer, bytes)
> - takes a pointer (of any type), increment it by |bytes| bytes, return a
> |void*|

Nit on the name: can we take .Utils out?  I think .Shared is already indication that we have utility code, we don't need death by namespacing.  (Actually, what's the point of .Shared?  Even if we have OS.Unix and OS.Windows, I think OS.Pointers or OS.Strings would be understood to be shared code...)

I realize this calls into question a whole host of other things outside of this bug.  Sorry about that.

> OS.Shared.PtrType.prototype.offsetBy(ptr or ArrayBuffer, index)
> - takes a pointer (of any type), increment it by |index * this.size| bytes,
> return a pointer of |this| type;
> - special case for |void*|, which replaces |this.size| with 1.
> 
> The main role of the first function being to implement the method.

So with this, we'd have:

Type.uint16_t.in_ptr.offsetBy(voidPtr, 2); /* (uint16_t*)((uint8_t*)voidPtr + 4) */
Type.uint32_t.in_ptr.offsetBy(buffer, 3); /* (uint32_t*)((uint8_t*)&buffer[0] + 12) */

correct?  And maybe something like (apologies if this is not 100% correct):

typedPointer.type.offsetBy(typedPointer, 3); /* typedPointer + 3 */

This last one looks especially ugly.

I like the original offsetBy better.  It's possible my JS-fu is just not strong enough here, though.  Josh, do you have an opinion here?

Also going to CC JimB, as I trust both his JS-fu and his API insight more than my own.
Comment on attachment 646976 [details] [diff] [review]
offsetBy, v2

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

Just f+'ing this for now, since we still have to get offsetBy sorted.

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +202,5 @@
> +        * without length, etc.
> +        */
> +       get size() {
> +         return this.implementation.size;
> +       }

Eh...I don't like this; it's not immediately clear whether the size is the size of the pointer or the pointed-to type.  (I realize the documentation makes this clear.)  Let's drop this and inline in the lone caller; if we think we need it elsewhere, we can revisit.

@@ +832,5 @@
>  
>       /**
> +      * A bogus array type used to perform pointer arithmetics.
> +      */
> +     let gOffsetByType;

I like it!

@@ +835,5 @@
> +      */
> +     let gOffsetByType;
> +
> +     /**
> +      * Advance in a pointer (or ArrayBuffer) by a number of bytes.

"Advance in" doesn't read well and is incorrect, just drop the "in".

Also, ArrayBuffers are given passing references in this comment; the behavior of the function WRT ArrayBuffers should be more explicit, I think.

@@ +837,5 @@
> +
> +     /**
> +      * Advance in a pointer (or ArrayBuffer) by a number of bytes.
> +      *
> +      * @param {C void* | ArrayBuffer} pointer The reference pointer.

It doesn't have to be void*, does it?  Could stand a little clarification here.

@@ +844,5 @@
> +      * is advanced by |offset| items of type |type|. Otherwise,
> +      * the pointer is advanced by |offset| bytes.
> +      * @param {CType*} type Optionally, a type.
> +      *
> +      * @return {C void*} A pointer corresponding to pointer advanced

"...corresponding to |pointer|..."
Attachment #646976 - Flags: review?(nfroyd) → feedback+
Comment on attachment 646979 [details] [diff] [review]
Variant with auto-casting to type

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

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +849,2 @@
>        * by |offset| items of type |type| (or bytes, if |type| is not
> +      * provided). If |type| was provided, it has type |type*|, otherwise

Nit: "...provided, the returned pointer has..." to clearly indicate what "it" refers to.

@@ +874,5 @@
> +       if (type) {
> +         if (type instanceof Type) {
> +           return type.cast(addr);
> +         } else {
> +           return ctypes.cast(addr, type);

I don't see explicit pointer types here, which I'd expect to see given the documentation above.  Either the interface should be changed to require an explicit pointer type, or this code needs some tweaking.
Attachment #646979 - Flags: feedback?(nfroyd) → feedback+
Attached patch offsetBy, v3 (obsolete) — Splinter Review
Actually, I have toyed with this a little, and I have a new API to suggest. In a way, it makes the |type| argument compulsory, which simplifies things.

I think this is the best version so far. Could you tell me what you think of it?
Attachment #647540 - Flags: review?(nfroyd)
Comment on attachment 647541 [details] [diff] [review]
Companion testsuite matching v3

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

It looks like you attached bits of the serialization patch here instead of the tests.
Attachment #647541 - Flags: review?(nfroyd)
Attached patch Companion testsuite matching v3 (obsolete) — Splinter Review
Oops.
Attachment #647541 - Attachment is obsolete: true
Attachment #647552 - Flags: review?(nfroyd)
Comment on attachment 647552 [details] [diff] [review]
Companion testsuite matching v3

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

I'd like to see a test for void* arithmetic (whichever way we want it to work), and I think the naming could be improved, but this seems reasonable.  r=me with the added test and bits below addressed.

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +293,5 @@
> +  // Initialize one array
> +  let LENGTH = 1024;
> +  let buf = new ArrayBuffer(LENGTH);
> +  let view = new Uint8Array(buf);
> +  let i;

Nit: move this let into the |for| loops.

@@ +300,5 @@
> +  }
> +
> +  // Walk through the array with offsetBy
> +  for (i = 0; i < LENGTH; ++i) {
> +    let value = OS.Shared.Type.uint8_t.in_ptr.offsetBy(buf, i).contents;

Ow.my.eyes.  But I have discussed this elsewhere. :)
Attachment #647552 - Flags: review?(nfroyd) → review+
Comment on attachment 647540 [details] [diff] [review]
offsetBy, v3

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

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +224,3 @@
>         Type.call(this, name, implementation);
> +       if (targetType == null || !targetType instanceof Type) {
> +         throw new TypeError("PtrType expects as TargetType an instance of Type");

I find this phrasing a little awkward, maybe "targetType must be an instance of Type"?  Call stacks should take care of the PtrType-ness of the context.

@@ +224,5 @@
>         Type.call(this, name, implementation);
> +       if (targetType == null || !targetType instanceof Type) {
> +         throw new TypeError("PtrType expects as TargetType an instance of Type");
> +       }
> +       this.targetType = targetType;

I like this idea.  I think you should use defineProperty here for consistency with Type.  (I'm not going to look up the Type patches, but ISTR that you used defineProperty for Type.  If I'm wrong, feel free to ignore this.)

@@ +282,5 @@
> +     // A bogus array type used to perform pointer arithmetics
> +     let gOffsetByType;
> +
> +     // Advance a pointer by a number of bytes
> +     PtrType.prototype.offsetByBytes = function offsetByBytes(pointer, bytes) {

This method should be module-internal; people may get confused between offsetBy and offsetByBytes.

@@ +307,5 @@
> +      * This method implements adding an integer to a pointer in C.
> +      *
> +      * Example:
> +      * Type.uint16_t.offsetBy(ptr, 3)
> +      *  // returns a uint16_t* with the address ptr + 3 * 2 bytes

Nit: the example is wrong, since the offsetBy method is on PtrType, not Type.

Incidentally, this example illustrates problems with the approach:

- Too.much.sub.notation.to.get.to.offsetBy.  (Bad here, ow-my-eyes bad in the tests.)
- Requiring the user to match up types by default is burdensome.  It's good that you can say:

    Type.uint16_t.in_ptr.offsetBy(uint32Ptr, 3);

  to do the casting and whatnot, but you have to do that in the common case too:

    Type.uint32_t.in_ptr.offsetBy(uint32Ptr, 3);

  which doesn't seem helpful.  I think I liked the earlier, not-attached-to-a-type, offsetBy better.

@@ +314,5 @@
> +      * @param {number} num The number of items from which to advance.
> +      *
> +      * @return {C pointer} A C pointer with the same type as |this|,
> +      * whose address is the address of |pointer| + |num| * the size
> +      * of |this|.

Nit: "the size of the pointed-to type of |this|" is clearer, I think, since the current phrasing makes one think of |sizeof(this)| rather than what you're actually using, which is |sizeof(*this)|.  Or however you would prefer to phrase it.

@@ +326,5 @@
> +       }
> +       if (num == 0) {
> +         return this.cast(pointer);
> +       }
> +       let bytes = num * this.targetType.size;

You need to check that .size is defined.  Earlier implementations let you pun arithmetic on void* (which I think is a good thing); this version does strange things.  A test for arithmetic on void* would be a good idea.
Attachment #647540 - Flags: review?(nfroyd) → feedback+
(In reply to Nathan Froyd (:froydnj) from comment #27)
> Comment on attachment 647552 [details] [diff] [review]
> Companion testsuite matching v3
> 
> Review of attachment 647552 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to see a test for void* arithmetic (whichever way we want it to
> work), and I think the naming could be improved, but this seems reasonable. 
> r=me with the added test and bits below addressed.

With the latest API, offsetBy will raise an error on void* and offsetByBytes will work exactly as for other pointers.

> 
> ::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
> @@ +293,5 @@
> > +  // Initialize one array
> > +  let LENGTH = 1024;
> > +  let buf = new ArrayBuffer(LENGTH);
> > +  let view = new Uint8Array(buf);
> > +  let i;
> 
> Nit: move this let into the |for| loops.
> 
> @@ +300,5 @@
> > +  }
> > +
> > +  // Walk through the array with offsetBy
> > +  for (i = 0; i < LENGTH; ++i) {
> > +    let value = OS.Shared.Type.uint8_t.in_ptr.offsetBy(buf, i).contents;
> 
> Ow.my.eyes.  But I have discussed this elsewhere. :)

Just alias it to something else if you are not happy :)
(In reply to Nathan Froyd (:froydnj) from comment #28)
> Comment on attachment 647540 [details] [diff] [review]
> offsetBy, v3
> 
> Review of attachment 647540 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/osfile/osfile_shared_allthreads.jsm
> @@ +224,3 @@
> >         Type.call(this, name, implementation);
> > +       if (targetType == null || !targetType instanceof Type) {
> > +         throw new TypeError("PtrType expects as TargetType an instance of Type");
> 
> I find this phrasing a little awkward, maybe "targetType must be an instance
> of Type"?  Call stacks should take care of the PtrType-ness of the context.

Good point.

> @@ +224,5 @@
> >         Type.call(this, name, implementation);
> > +       if (targetType == null || !targetType instanceof Type) {
> > +         throw new TypeError("PtrType expects as TargetType an instance of Type");
> > +       }
> > +       this.targetType = targetType;
> 
> I like this idea.  I think you should use defineProperty here for
> consistency with Type.  (I'm not going to look up the Type patches, but ISTR
> that you used defineProperty for Type.  If I'm wrong, feel free to ignore
> this.)

Ok.

> @@ +282,5 @@
> > +     // A bogus array type used to perform pointer arithmetics
> > +     let gOffsetByType;
> > +
> > +     // Advance a pointer by a number of bytes
> > +     PtrType.prototype.offsetByBytes = function offsetByBytes(pointer, bytes) {
> 
> This method should be module-internal; people may get confused between
> offsetBy and offsetByBytes.

Actually, I think that both are useful. Plus one of them does not work on void*.

> 
> @@ +307,5 @@
> > +      * This method implements adding an integer to a pointer in C.
> > +      *
> > +      * Example:
> > +      * Type.uint16_t.offsetBy(ptr, 3)
> > +      *  // returns a uint16_t* with the address ptr + 3 * 2 bytes
> 
> Nit: the example is wrong, since the offsetBy method is on PtrType, not Type.

Ah, thanks, I forgot the pointer.

> Incidentally, this example illustrates problems with the approach:
> 
> - Too.much.sub.notation.to.get.to.offsetBy.  (Bad here, ow-my-eyes bad in
> the tests.)

Yeah, but that's because I have not aliased anything. In practice you will probably do something along the lines of

let ptrType = Type.uint16_t.in_ptr;
ptrType.offsetBy(myPtr, 3);

> - Requiring the user to match up types by default is burdensome.  It's good
> that you can say:
> 
>     Type.uint16_t.in_ptr.offsetBy(uint32Ptr, 3);
> 
>   to do the casting and whatnot, but you have to do that in the common case
> too:
> 
>     Type.uint32_t.in_ptr.offsetBy(uint32Ptr, 3);
> 
>   which doesn't seem helpful.  I think I liked the earlier,
> not-attached-to-a-type, offsetBy better.

I personally prefer this one, as it removes any ambiguity. Otherwise, you have to play guess-how-this-works-with-optional-types, which is a very bad idea for system-level programming (and even worse for pointer arithmetics).

> 
> @@ +314,5 @@
> > +      * @param {number} num The number of items from which to advance.
> > +      *
> > +      * @return {C pointer} A C pointer with the same type as |this|,
> > +      * whose address is the address of |pointer| + |num| * the size
> > +      * of |this|.
> 
> Nit: "the size of the pointed-to type of |this|" is clearer, I think, since
> the current phrasing makes one think of |sizeof(this)| rather than what
> you're actually using, which is |sizeof(*this)|.  Or however you would
> prefer to phrase it.

Good point.

> 
> @@ +326,5 @@
> > +       }
> > +       if (num == 0) {
> > +         return this.cast(pointer);
> > +       }
> > +       let bytes = num * this.targetType.size;
> 
> You need to check that .size is defined.  Earlier implementations let you
> pun arithmetic on void* (which I think is a good thing); this version does
> strange things.  A test for arithmetic on void* would be a good idea.

Actually, I rely on js-ctypes launching an exception, but you are right, I should do it myself.
Attached patch offsetBy, v4 (obsolete) — Splinter Review
Applied feedback.
Attachment #647540 - Attachment is obsolete: true
Attachment #649126 - Flags: review?(nfroyd)
Attached patch Companion testsuite, v4 (obsolete) — Splinter Review
As requested, adding a test for voidptr_t walking.
Attachment #646609 - Attachment is obsolete: true
Attachment #646976 - Attachment is obsolete: true
Attachment #646979 - Attachment is obsolete: true
Attachment #647552 - Attachment is obsolete: true
Attachment #649127 - Flags: review?(nfroyd)
Comment on attachment 649127 [details] [diff] [review]
Companion testsuite, v4

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

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +324,5 @@
> +      break;
> +    }
> +  }
> +
> +  // Test that we cannot walk with 0 bytes

Just "Test that we cannot successfully call offsetBy on a void pointer." is sufficient.
Attachment #649127 - Flags: review?(nfroyd) → review+
(In reply to David Rajchenbach Teller from comment #30)
> > - Requiring the user to match up types by default is burdensome.  It's good
> > that you can say:
> > 
> >     Type.uint16_t.in_ptr.offsetBy(uint32Ptr, 3);
> > 
> >   to do the casting and whatnot, but you have to do that in the common case
> > too:
> > 
> >     Type.uint32_t.in_ptr.offsetBy(uint32Ptr, 3);
> > 
> >   which doesn't seem helpful.  I think I liked the earlier,
> > not-attached-to-a-type, offsetBy better.
> 
> I personally prefer this one, as it removes any ambiguity. Otherwise, you
> have to play guess-how-this-works-with-optional-types, which is a very bad
> idea for system-level programming (and even worse for pointer arithmetics).

I will note that my original proposal (comment 8) only took optional types for ArrayBuffer arguments.  You can get by without the optional type argument in the ArrayBuffer case and for the pointer case, it's only a nice-to-have anyway.

Regardless, I don't see how removing ambiguity is beneficial here compared to:

  base = OS.Utils.offsetBy(myPtr, 2); /* base = myPtr + 2 in C */

Keeping track of the types of base and myPtr in this example is no more complicated than what JS programmers do all the time, every day.

If we have to tweak ctypes to be able to write the above, I think that's reasonable, though a ctypes expert might enlighten me as to why we don't want to do that.
(In reply to Nathan Froyd (:froydnj) from comment #34)
> I will note that my original proposal (comment 8) only took optional types
> for ArrayBuffer arguments.  You can get by without the optional type
> argument in the ArrayBuffer case and for the pointer case, it's only a
> nice-to-have anyway.

Ah, I lost track of the fact that this was only for ArrayBuffer.
I am (now) clearly against optional type argument for pointers, btw.

> Regardless, I don't see how removing ambiguity is beneficial here compared
> to:
> 
>   base = OS.Utils.offsetBy(myPtr, 2); /* base = myPtr + 2 in C */
> 
> Keeping track of the types of base and myPtr in this example is no more
> complicated than what JS programmers do all the time, every day.

My bad, I mentioned ambiguity for
  OS.Utils.offsetBy(myPtr, 2, someType)


> If we have to tweak ctypes to be able to write the above, I think that's
> reasonable, though a ctypes expert might enlighten me as to why we don't
> want to do that.

Indeed, this sounds reasonable. Right now, the main obstacle is that there is no getter for the C type of an object.

Let me suggest the following:
- for the moment, we use this patch more or less as it is, keeping in mind that we will want a better version sooner or later;
- open a followup js-ctypes bug for getting the C type of an object;
- open a followup OS.File bug for OS.Utils.offsetBy that works by magic.
(In reply to David Rajchenbach Teller from comment #35)
> (In reply to Nathan Froyd (:froydnj) from comment #34)
> > I will note that my original proposal (comment 8) only took optional types
> > for ArrayBuffer arguments.  You can get by without the optional type
> > argument in the ArrayBuffer case and for the pointer case, it's only a
> > nice-to-have anyway.
> 
> Ah, I lost track of the fact that this was only for ArrayBuffer.
> I am (now) clearly against optional type argument for pointers, btw.

I may have insinuated that we should add the optional argument for pointers, too.  Your stand against optional type arguments is correct; my earlier attempts were too cute.

> > If we have to tweak ctypes to be able to write the above, I think that's
> > reasonable, though a ctypes expert might enlighten me as to why we don't
> > want to do that.
> 
> Indeed, this sounds reasonable. Right now, the main obstacle is that there
> is no getter for the C type of an object.

This is just obj.constructor, right?  Except that doesn't work quite correctly for data with finalizers?

> Let me suggest the following:
> - for the moment, we use this patch more or less as it is, keeping in mind
> that we will want a better version sooner or later;
> - open a followup js-ctypes bug for getting the C type of an object;
> - open a followup OS.File bug for OS.Utils.offsetBy that works by magic.

I am reluctant to follow this plan.  Followup bugs don't always get fixed unless there is some urgency to them.  And there is no urgency here.

I am also a little wary about breaking offsetBy once it's out in the wild, though perhaps (a) offsetBy usage will not be widespread; and (b) people should expect some breakage in OS.File for a little while.  And this would be a loud breakage.

How about this alternative plan: file the js-ctypes bug, make it block this one.  Work to fix it, discuss with js-ctypes folks as necessary.  If you cannot get it resolved by next Friday (17 August), then we'll go ahead and follow your plan outlined above.  (I'm suggesting next Friday in an attempt to hold open the possibility that the bulk of OS.File bits can be in by the next uplift, 27 August)  Does that sound reasonable to you?
(In reply to Nathan Froyd (:froydnj) from comment #36)
> I may have insinuated that we should add the optional argument for pointers,
> too.  Your stand against optional type arguments is correct; my earlier
> attempts were too cute.

We're moving forward :)

> > > If we have to tweak ctypes to be able to write the above, I think that's
> > > reasonable, though a ctypes expert might enlighten me as to why we don't
> > > want to do that.
> > 
> > Indeed, this sounds reasonable. Right now, the main obstacle is that there
> > is no getter for the C type of an object.
> 
> This is just obj.constructor, right?  Except that doesn't work quite
> correctly for data with finalizers?

/me uses head on wall

Yes, obj.constructor seems to work! The constructor is so often improperly defined that I forgot to try it. Thank you very much.

With this breakthrough, I believe can make it work for most data structures (with the exception of finalizers) immediately. I can extend this to finalizers later.

> I am also a little wary about breaking offsetBy once it's out in the wild,
> though perhaps (a) offsetBy usage will not be widespread; and (b) people
> should expect some breakage in OS.File for a little while.  And this would
> be a loud breakage.

Actually, I was thinking about having two |offsetBy| (one attached to PtrType.prototype and one attached to Utils.Pointers), but your remark on obj.constructor most likely makes this irrelevant.

> How about this alternative plan: file the js-ctypes bug, make it block this
> one.  Work to fix it, discuss with js-ctypes folks as necessary.  If you
> cannot get it resolved by next Friday (17 August), then we'll go ahead and
> follow your plan outlined above.  (I'm suggesting next Friday in an attempt
> to hold open the possibility that the bulk of OS.File bits can be in by the
> next uplift, 27 August)  Does that sound reasonable to you?

Let me suggest this variant:
- implement immediately (and land) a version that does not work with finalizers;
- file a followup js-ctypes bug for finalizers;
- file a followup OS.File bug for finalizers.
Comment on attachment 649126 [details] [diff] [review]
offsetBy, v4

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

(In reply to David Rajchenbach Teller from comment #37)
> Let me suggest this variant:
> - implement immediately (and land) a version that does not work with
> finalizers;
> - file a followup js-ctypes bug for finalizers;
> - file a followup OS.File bug for finalizers.

That works for me.  Clearing r? flag on this patch, then.
Attachment #649126 - Flags: review?(nfroyd)
Attached patch offsetBy, v5 (obsolete) — Splinter Review
Here we go. I have left two versions: one that extracts the type from the pointer (and consequently does not work on non-pointers) and one that accepts a very prominent type information (and works on ArrayBuffers, too).
Attachment #649126 - Attachment is obsolete: true
Attachment #650312 - Flags: review?(nfroyd)
No longer blocks: 771927
Attachment #650312 - Attachment is patch: true
Comment on attachment 650312 [details] [diff] [review]
offsetBy, v5

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

r=me with changes below.

::: toolkit/components/osfile/osfile_shared_allthreads.jsm
@@ +227,5 @@
> +       }
> +       Object.defineProperty(this, "targetType", {
> +         /**
> +          * The type of values targeted by this pointer type.
> +          */

I think it's slightly better to move this comment to before the defineProperty call and to make it an //-style comment.

@@ +291,5 @@
> +      *
> +      * Example:
> +      *   // ptr is any pointer or an ArrayBuffer
> +      *   uint16_t.offsetBy(ptr, 3)
> +      *  // returns a uint16_t* with the address ptr + 3 * 2 bytes

This explanation is confused; offsetBy is on PtrType, of which type uint16_t is probably not.  Please correct it.  (I am getting a little skeptical of the usage of this now that you've made this mistake twice! :)

Your review request seems to ask for a referendum on the goodness of one version versus two.  I say delete this version; if implementing bits of OS.File becomes burdensome without it (or something like it), we can revisit this decision.

@@ +818,5 @@
>  
>       exports.OS.Shared.Utils = {};
>  
>       let Strings = exports.OS.Shared.Utils.Strings = {};
>       let Pointers = exports.OS.Shared.Utils.Pointers = {};

I think this is death.by.namespacing.  Please just put offsetBy in OS.Shared.Utils, even if OS.Shared would be better.  We can have a conversation about what to do with OS.Shared.Utils.Strings later. :)

@@ -825,5 @@
>       };
>  
> -     exports.OS.Shared.Utils = { Strings: Strings };
> -
> -

Did you mean to delete all this?
Attachment #650312 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #40)
> Comment on attachment 650312 [details] [diff] [review]
> offsetBy, v5
> 
> Review of attachment 650312 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with changes below.
> 
> ::: toolkit/components/osfile/osfile_shared_allthreads.jsm
> @@ +227,5 @@
> > +       }
> > +       Object.defineProperty(this, "targetType", {
> > +         /**
> > +          * The type of values targeted by this pointer type.
> > +          */
> 
> I think it's slightly better to move this comment to before the
> defineProperty call and to make it an //-style comment.

Moving it up is a good idea. However, I intentionally made this a /*-style comment as this is basically a public field, hence part of the API. I generally attempt to keep //-style comments to implementation details. So, if you do not mind, I would rather keep this a /*-style comment.


> 
> @@ +291,5 @@
> > +      *
> > +      * Example:
> > +      *   // ptr is any pointer or an ArrayBuffer
> > +      *   uint16_t.offsetBy(ptr, 3)
> > +      *  // returns a uint16_t* with the address ptr + 3 * 2 bytes
> 
> This explanation is confused; offsetBy is on PtrType, of which type uint16_t
> is probably not.  Please correct it.  (I am getting a little skeptical of
> the usage of this now that you've made this mistake twice! :)

Oops.

> Your review request seems to ask for a referendum on the goodness of one
> version versus two.  I say delete this version; if implementing bits of
> OS.File becomes burdensome without it (or something like it), we can revisit
> this decision.

The only reason I keep this version is that it works on ArrayBuffer, which is a little easier on users than having to find out that they can construct a pointer from an ArrayBuffer. However, since the only real scenario for this is readAll/writeAll, that's not a big deal.

> 
> @@ +818,5 @@
> >  
> >       exports.OS.Shared.Utils = {};
> >  
> >       let Strings = exports.OS.Shared.Utils.Strings = {};
> >       let Pointers = exports.OS.Shared.Utils.Pointers = {};
> 
> I think this is death.by.namespacing.  Please just put offsetBy in
> OS.Shared.Utils, even if OS.Shared would be better.  We can have a
> conversation about what to do with OS.Shared.Utils.Strings later. :)

ok :)

> @@ -825,5 @@
> >       };
> >  
> > -     exports.OS.Shared.Utils = { Strings: Strings };
> > -
> > -
> 
> Did you mean to delete all this?

Yes, it's a trivial cleanup.
Attached patch offsetBy, v6Splinter Review
Attachment #650312 - Attachment is obsolete: true
Attachment #649127 - Attachment is obsolete: true
Attachment #651730 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/acbba69667d8
https://hg.mozilla.org/mozilla-central/rev/e953b3f9ddd6
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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: