578700 - (harmony:typedobjects) [meta] Harmony typed objects (nés binary data)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task)

Description

[Dave Herman [:dherman]]

Binary data objects for JS, modeled after js-ctypes, and hopefully useful for WebGL and others (File API, XHR, pure JS crypto algorithms, ...). Jonas and I are trying to sketch this out.

When there are wiki specs available on the TC39 page, I'll link to them in the URL field.

Dave

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Specification: http://wiki.ecmascript.org/doku.php?id=harmony:binary_data

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I'm working on this - patch queue at http://hg.mozilla.org/users/nsm.nikhil_gmail.com/binarydata/

Comment 3

[David Mandelin [:dmandelin]]

I believe Nikhil doesn't have time for this at the moment, so clearing assignee.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

I've attached the first binary data implementation as a series of patches.
I probably should've done this in more granular pieces over the past few weeks
but unfortunately didn't. This implementation does not have JIT optimizations.
The implementation tries to stay true to [1], but there may be bugs. Since the
specification isn't complete, behaviour may change.

[1]: http://wiki.ecmascript.org/doku.php?id=harmony:binary_data_semantics

I'll now describe the implementation:

- The numeric types (uint8, float32 etc.) are simple functions backed by the
  NumericType class methods. The functions hold their type in an internal slot.
  Since numeric types instances are never exposed to JS directly (they always
  get converted to Numbers), there is no JSObject involved.

- The ArrayType and StructType classes are considered 'complex' and functions
  operating on both use that nomenclature in the source. Instances of both
  types have a slot (SLOT_MEMSIZE) holding the internal true size in bytes of
  the instance (which may be different from the "bytes" JavaScript size). SLOT_ALIGN
  contains the alignment.
  
  An instance of ArrayType, created by:
         var A = new ArrayType(uint8, 10);
  is backed by a JSObject and operations are contained in the ArrayType C++
  class in the source. ArrayType instances have an "elementType" (uint8 in the
  case of A) which is used to determine SLOT_MEMSIZE, SLOT_ALIGN and used when
  performing operations.
  
  An instance of StructType, created by:
         var S = new StructType({field1: type1, field2: type2, ...});
  is backed by a JSObject and operations are contained in the StructType C++
  class.  StructType maintains an internal hash (in JSObject's private field)
  containing the mappings from field name -> { type, offset }, where offset is
  the real memory offset of the field calculated at creation time based on
  sizes and alignment of members and padding (StructType::layout).

- Instances of instances of ArrayType/StructType, i.e:
        var a = new A()
  are *non-native* JSObjects, where the SLOT_DATATYPE is a pointer to the JSObject
  representing the type (i.e. a.SLOT_DATATYPE == A's JSObject). The actual block
  of memory is held in the private field of the JSObject.

- The code for numeric type checks and memory alignment calculation is taken
  from js-ctypes as required. In particular, the range checks in binary data
  may not be accurate, but I don't have much understanding of this area and
  feedback is appreciated.

- In the case of accessing properties, where the property (index or field) is
  a complex type, the BinaryArray/BinaryStruct instance records the real owner
  of the memory and marks it as alive to prevent the owner getting GC'ed. For
  example:
         var DIM_2 = new ArrayType(new ArrayType(int32, 3), 3);
         var mat = new DIM_2([[1,2,3], [4,5,6], [7,8,9]])
         var row1 = mat[0]
  `mat` is the owner of the memory referred to in it's private field. It's
  SLOT_BLOCKREFOWNER is NullValue().  `row1` private refers to an offset in the
  memory owned by `mat`, so row1.SLOT_BLOCKREFOWNER is `mat` and
  BinaryArray::obj_trace will mark `mat` to keep the memory around.

  This approach works and seems to cover the updateRef implementation as well
  (not done yet).
  Would a reference counting approach be a better fit?

This is a WIP, and changes may be followed at [2]. All feedback along the way
is most welcome.

[2]: http://hg.mozilla.org/users/nsm.nikhil_gmail.com/binarydata-take2/

Comment 5

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - Layout the type heirarchy

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - numeric binary data implementation

jorendorff: I've used range checking code from js-ctypes. So I would really appreciate it if you could give this a look and see if I've made any mistakes with casting, conversion and limits checks.

Comment 7

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 3 - ArrayType and BinaryArray implementation

Comment 8

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 4 - StructType and BinaryStruct implementation

Comment 9

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 5 - Implement the utility methods in the spec

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 6 - Memory management and GC related fixes

Comment 11

[:Benjamin Peterson]

A few broad comments:
- jsbinarydata.cpp should probably be in the newer place for these types of things js/src/builtin/
- Use the internal JS API. For example JSVAL_IS_PRIMITIVE(v) should be v.isPrimitive().
- Use bool instead of JSBool except when it is needed to avoid compilation failures.

Comment 12

[Luke Wagner [:luke]]

I'm afraid I'm booked for the next few months so I'm probably not the reviewer you want.  dmandelin might be able to help you find a reviewer.

Comment 13

[Brendan Eich [:brendan]]

See also bug 749786 -- int64/uint64 value types. Happy to track this bug to reuse template-ology.

/be

Comment 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - Layout the type heirarchy

Un-bitrotted the patches.

Comment 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - numeric binary data implementation

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 3 - ArrayType and BinaryArray implementation

Comment 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 4 - StructType and BinaryStruct implementation

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 5 - Implement the utility methods in the spec

Comment 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 6 - Memory management and GC related fixes

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 1 - Layout the type heirarchy

Update to work with latest Spidermonkey, especially rooting changes.

Comment 21

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - numeric binary data implementation

Comment 22

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - numeric binary data implementation

Comment 23

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 3 - ArrayType and BinaryArray implementation

Comment 24

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 4 - StructType and BinaryStruct implementation

Comment 25

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 5 - Implement the utility methods in the spec

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 6 - Memory management and GC related fixes

Comment 27

[Niko Matsakis [:nmatsakis]]

Comment on attachment 752284 [details] [diff] [review]
Part 1 - Layout the type heirarchy

Adopting review (with permission).

Comment 28

[Niko Matsakis [:nmatsakis]]

Comment on attachment 752286 [details] [diff] [review]
Part 2 - numeric binary data implementation

Adopting review (with permission).

Comment 29

[Niko Matsakis [:nmatsakis]]

Comment on attachment 752284 [details] [diff] [review]
Part 1 - Layout the type heirarchy

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

Looks good, but needs to be updated so as not to read
from locations mutable by the user (see comments below
for details).

::: js/src/jsbinarydata.cpp
@@ +79,5 @@
> +SetupComplexHeirarchy(JSContext *cx, HandleObject global, HandleObject complexObject)
> +{
> +    // get the 'Type' function
> +    Value TypeVal;
> +    if (!JS_GetProperty(cx, global, "Type", &TypeVal))

This code simply reads the global object's properties, which are vulnerable to being changed by the end user. I think we want to use a pattern more like what is used in `builtin/Intl.cpp` [0], which amounts to a call to getOrCreateObject() [1].


[0] http://dxr.mozilla.org/mozilla-central/js/src/builtin/Intl.cpp#l700
[1] http://dxr.mozilla.org/mozilla-central/js/src/vm/GlobalObject.h#l338

@@ +83,5 @@
> +    if (!JS_GetProperty(cx, global, "Type", &TypeVal))
> +        return NULL;
> +
> +    RootedObject TypeFunObj(cx);
> +    if (!TypeVal.isPrimitive())

Should this be an assertion?

@@ +94,5 @@
> +        return NULL;
> +
> +    // get the 'Data' function
> +    jsval DataVal;
> +    if (!JS_GetProperty(cx, global, "Data", &DataVal))

As before, avoid reading global object properties in this way.

@@ +98,5 @@
> +    if (!JS_GetProperty(cx, global, "Data", &DataVal))
> +        return NULL;
> +
> +    RootedObject DataFunObj(cx);
> +    if (!DataVal.isPrimitive())

Should this be an assertion?

@@ +118,5 @@
> +    if (!LinkConstructorAndPrototype(cx, prototypeObj, prototypePrototypeObj))
> +        return NULL;
> +
> +    RootedValue DataPrototypeVal(cx);
> +    if (!JSObject::getProperty(cx, DataFunObj, DataFunObj, cx->names().classPrototype, &DataPrototypeVal))

As before, avoid reading global object properties in this way.

Comment 30

[Niko Matsakis [:nmatsakis]]

Comment on attachment 752286 [details] [diff] [review]
Part 2 - numeric binary data implementation

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

This code looks basically all right.

::: js/src/jsbinarydata.cpp
@@ +26,5 @@
>      return ReportIsNotFunction(cx, *vp);
>  }
>  
> +template <typename Domain, typename Input>
> +bool InRange(Input x)

Nit: I believe the coding conventions here and elsewhere in this file suggest that the return type should be on a line of its own.

@@ +64,5 @@
> +        return true;
> +    }
> +
> +    if (val.isNumber()) {
> +        // NOTE is this the right way to do it?

It's a little unclear what the desired semantics are here (in the spec as well). This looks...reasonable, we can revisit as needed.

@@ +80,5 @@
> +            }
> +        }
> +    }
> +
> +    // TODO conditional processing for (U)Int64

UInt64/Int64 is not currently part of the spec, remove TODO.

@@ +105,5 @@
> +        *casted = (T) val.toNumber();
> +        return true;
> +    }
> +
> +    // TODO val is a js-ctypes (U)Int64

No need for this, uint64 is deferred for now.

@@ +126,5 @@
> +template <typename T>
> +JSBool NumericType<T>::call(JSContext *cx, unsigned argc, Value *vp)
> +{
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    if (args.length() < 1) // TODO should we raise error?

Yes, raise TypeError with JSMSG_MORE_ARGS_NEEDED

@@ +140,5 @@
> +        JS_free(cx, (void *) fnName);
> +        return false;
> +    }
> +
> +    // TODO reify

I take it this TODO means "we should be returning a Data instance here, not the raw value"? Perhaps this comes in a later patch?

Comment 31

[Niko Matsakis [:nmatsakis]]

Comment on attachment 752287 [details] [diff] [review]
Part 3 - ArrayType and BinaryArray implementation

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

::: js/src/jsbinarydata.cpp
@@ +357,5 @@
> +    JS_ASSERT(elementTypeVal.isObject());
> +    return elementTypeVal.toObjectOrNull();
> +}
> +
> +// currently memory leaks

is this addressed in a later patch?

@@ +402,5 @@
> +
> +    for (int32_t i = 0; i < fromLen; i++) {
> +        RootedValue fromElem(cx);
> +        if (!JSObject::getElement(cx, valRooted, valRooted, i, &fromElem))
> +            continue; // TODO should we abort here?

If false is returned, then an exception has been thrown, so we should propagate that and return false here.

@@ +652,5 @@
> +{
> +    JS_ASSERT(IsBinaryArray(obj));
> +
> +    uint32_t index;
> +    if (js_IdIsIndex(id, &index) && index < ArrayType::length(cx, GetType(obj))) { // TODO check length

What does this TODO mean? It looks like you are checking the length.

@@ +1030,5 @@
> +    return false;
> +}
> +
> +static bool
> +ConvertAndCopyTo(JSContext *cx, JSObject *type, HandleValue from, void *mem)

I think that you should merge convert and ConvertAndCopyTo into one function, perhaps named "convertInto" or else just "convertAndCopyTo". You sort of this in attachment 752294 [details] [diff] [review]. It is

@@ +1164,5 @@
>          return NULL;
>  
>      // ArrayType.prototype.repeat
>      RootedValue ArrayTypePrototypeVal(cx);
> +    JSObject::getProperty(cx, ArrayTypeFun, ArrayTypeFun, cx->names().classPrototype, &ArrayTypePrototypeVal);

As before, do not read user-mutable data here.

@@ +1172,4 @@
>          return NULL;
>  
> +    RootedValue ArrayTypePrototypePrototypeVal(cx);
> +    JSObject::getProperty(cx, ArrayTypePrototypeObj, ArrayTypePrototypeObj, cx->names().classPrototype, &ArrayTypePrototypePrototypeVal);

As before, do not read user-mutable data here.

Comment 32

[Niko Matsakis [:nmatsakis]]

Comment on attachment 752288 [details] [diff] [review]
Part 4 - StructType and BinaryStruct implementation

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

::: js/src/jsbinarydata.cpp
@@ +207,5 @@
> +    RootedObject global(cx, cx->compartment->maybeGlobal());
> +    RootedValue complexTypeVal(cx);
> +    RootedValue complexTypePrototypeVal(cx);
> +    RootedValue complexTypePrototypePrototypeVal(cx);
> +    if (!JSObject::getProperty(cx, global, global, complexTypeName, &complexTypeVal))

As before, I do not think that we want to read from user-modifiable state here.

@@ +211,5 @@
> +    if (!JSObject::getProperty(cx, global, global, complexTypeName, &complexTypeVal))
> +        return NULL;
> +
> +    RootedObject complexTypeObj(cx, complexTypeVal.toObjectOrNull());
> +    if (!JSObject::getProperty(cx, complexTypeObj, complexTypeObj, cx->names().classPrototype, &complexTypePrototypeVal))

As before, I do not think that we want to read from user-modifiable state here.

@@ +215,5 @@
> +    if (!JSObject::getProperty(cx, complexTypeObj, complexTypeObj, cx->names().classPrototype, &complexTypePrototypeVal))
> +        return NULL;
> +
> +    RootedObject complexTypePrototypeObj(cx, complexTypePrototypeVal.toObjectOrNull());
> +    if (!JSObject::getProperty(cx, complexTypePrototypeObj, complexTypePrototypeObj, cx->names().classPrototype, &complexTypePrototypePrototypeVal))

As before, I do not think that we want to read from user-modifiable state here.

@@ +586,5 @@
> +    StringBuffer contents(cx);
> +    contents.append("ArrayType(");
> +
> +    Value elementStringVal;
> +    if (!JS_CallFunctionName(cx, elementType(cx, thisObj), "toString", 0, NULL, &elementStringVal))

Use JS_ValueToString or js::ToString<CanGC>

@@ +1271,5 @@
> +        FieldInfo fieldInfo = r.front().value;
> +
> +        RootedValue fromProp(cx);
> +        if (!JSObject::getProperty(cx, valRooted, valRooted, AtomizeString<CanGC>(cx, r.front().key)->asPropertyName(), &fromProp))
> +            continue; // TODO should we abort here?

Yes, propagate the error.

@@ +1274,5 @@
> +        if (!JSObject::getProperty(cx, valRooted, valRooted, AtomizeString<CanGC>(cx, r.front().key)->asPropertyName(), &fromProp))
> +            continue; // TODO should we abort here?
> +
> +        if (ConvertAndCopyTo(cx, fieldInfo.type, fromProp, (uint8_t *) block + fieldInfo.offset))
> +            continue; // TODO what to do?

I don't understand this comment. Propagate fatal errors to be sure.

@@ +1367,5 @@
> +        RootedValue fieldTypeVal(cx);
> +        if (!JSObject::getProperty(cx, fields, fields, ToAtom<CanGC>(cx, IdToValue(fieldNames[i]))->asPropertyName(), &fieldTypeVal))
> +            return false;
> +        Value fieldStringVal;
> +        if (!JS_CallFunctionName(cx, fieldTypeVal.toObjectOrNull(), "toString", 0, NULL, &fieldStringVal))

As before, use JS_ValueToString or one of the other built-ins.

@@ +1906,5 @@
>      if (!SetupComplexHeirarchy(cx, obj, JSProto_StructType, StructTypeFun))
>          return NULL;
>  
> +    RootedValue StructTypePrototypeVal(cx);
> +    JSObject::getProperty(cx, StructTypeFun, StructTypeFun, cx->names().classPrototype, &StructTypePrototypeVal);

Don't read user-modifiable state.

Comment 33

[Niko Matsakis [:nmatsakis]]

Comment on attachment 752294 [details] [diff] [review]
Part 6 - Memory management and GC related fixes

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

::: js/src/jsbinarydata.cpp
@@ +476,5 @@
>          RootedValue fromElem(cx);
>          if (!JSObject::getElement(cx, valRooted, valRooted, i, &fromElem))
>              continue; // TODO should we abort here? a JS array can have holes
>  
> +        if (!ConvertAndCopyTo(cx, elementType, fromElem, (*blockref) + offsetMult * i))

This doesn't work. Here `blockref` is acting as a kind of "in-out" parameter---that is, in this case, you copy into blockref, but in the previous case (where the value being converted was already a binary data object) you are just setting blockref. I think the correct thing to do here is to remove `convert` and `ConvertAndCopyTo` and just have `convertInto` that is basically equivalent to `ConvertAndCopyTo`. This is just a transform to 'destination passing style', should work fine.

Comment 34

[Niko Matsakis [:nmatsakis]]

The code looks mostly correct, but there is some iteration necessary. I'm going to read a bit more in depth in the meantime, but the following two changes that are clearly needed:

- Don't read user-modifiable state when setting up the type hierarchies, converting to strings, and so forth

- Convert the `convert` routine to destination-passing-style (like `ConvertAndCopyTo` today). This addresses the issues of leaks and the confusing in-out parameter.

Another thought I had is that it would be good to convert as much of this C++ code into self-hosted code as we can. However, I have to do some prototyping and investigation to determine how possible that is. One blocking issue is having to deal with array element accesses (`x[i]`), which is currently not possible without diverting into C++. But maybe we can land move as much of the logic into self-hosted code as possible, and solve the `x[i]` issue independently. I imagine we could get pretty far with just a 'memcpy' intrinsic or something similar.

We probably want to set this code up so that it is only enabled in nightly builds as well, at least until it stabilizes.

Nikhil: Please ping me on IRC if you have any questions.

Comment 35

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1: Layout the type heirarchy

@nmatsakis: Is this along the lines of your comments?

Comment 36

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Part 2 - numeric binary data implementation

The reify TODO is indeed fixed in a later patch if I remember correctly. (Or should be if it is not)

Comment 37

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

:nmatsakis, please hold off from reviewing on Friday. I'll have some significantly cleaned up patches up by the end of Friday. Thanks!

Comment 38

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1: Layout the type heirarchy

The upcoming series of patches cleans up things, moves the memory management patch into the array type and struct type patches and fixes global object issues pointed out before. Each patch should now produce a working, testable build.

Comment 39

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 2: numeric binary data implementation

Comment 40

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 3: ArrayType and BinaryArray implementation

Comment 41

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 4: StructType and BinaryStruct implementation

Comment 42

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Comment on attachment 752291 [details] [diff] [review]
Part 5 - Implement the utility methods in the spec

Obsoleting since it won't build against latest tree and set of patches. New one coming up later.

Comment 43

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 5: Implement the utility methods in the spec

subarray is commented since the spec has erased it and there are compilation issues.

Comment 44

[Niko Matsakis [:nmatsakis]]

Comment on attachment 769191 [details] [diff] [review]
Patch 4: StructType and BinaryStruct implementation

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

::: js/src/jsbinarydata.cpp
@@ +165,5 @@
> +static size_t
> +GetAlignedSize(size_t size, size_t align)
> +{
> +    JS_ASSERT(align != 0 && (align & (align - 1)) == 0);
> +    return ((size - 1) | (align - 1)) + 1;

Cute bit shifting algorithm. I hadn't seen that before.

@@ +1309,5 @@
> +    JSObject *type;
> +    size_t offset;
> +};
> +
> +typedef HashMap<JSString *, FieldInfo> FieldMap;

I think it would be preferable to use an association list (i.e., just a vector of FieldInfo structs, where you add in a `jsid` to the FieldInfo struct) here. This would preserve ordering of the fields for use toString() and IsSameType(). Lookup would probably be faster unless there are a large number of fields, though we'd have to measure to find the exact break-even point. Certainly using a jsid rather than JSString guarantees interning and should make equality comparison very efficient.

@@ +1354,5 @@
> +        if (!JS_GetProperty(cx, fieldType, "bytes", &fieldTypeBytes))
> +            return false;
> +
> +        JS_ASSERT(fieldTypeBytes.isInt32());
> +        structByteSize += fieldTypeBytes.toInt32();

We may not be able to assume int32.

@@ +1413,5 @@
> +
> +    FieldMap *fieldMap = static_cast<FieldMap *>(exemplar->getPrivate());
> +
> +    if (ownProps.length() != fieldMap->count())
> +        return false;

report an error here

@@ +1417,5 @@
> +        return false;
> +
> +    for (int i = 0; i < ownProps.length(); i++) {
> +        if (!fieldMap->lookup(IdToString(cx, ownProps[i])))
> +            return false;

report an error here

@@ +1434,5 @@
> +
> +        RootedObject fieldType(cx, fieldInfo.type);
> +        if (!ConvertAndCopyTo(cx, fieldType, fromProp,
> +                              (uint8_t *) mem + fieldInfo.offset)) {
> +            ReportTypeError(cx, fromProp, "conversion error");

I believe that the error should be reported by the recursive call to `ConvertAndCopyTo`

@@ +1465,5 @@
> +        return NULL;
> +    }
> +
> +    if (!JS_DefineProperty(cx, obj, "fields",
> +                           ObjectValue(*fields), NULL, NULL,

It seems like we should copy this list, since it is ultimately derived from user-input, or else store a "parsed" version and regenerate it.

@@ +1657,5 @@
> +BinaryStruct::obj_lookupGeneric(JSContext *cx, HandleObject obj, HandleId id,
> +                                MutableHandleObject objp,
> +                                MutableHandleShape propp)
> +{
> +	JS_ASSERT(0);

As stated elsewhere (can't remember which patch), I think the correct thing to do is just to set the relevant jsop to NULL here, which will cause dispatch to flow to the generic implementation instead.

@@ +1666,5 @@
> +                                 HandlePropertyName name,
> +                                 MutableHandleObject objp,
> +                                 MutableHandleShape propp)
> +{
> +	JS_ASSERT(0);

As before, set to NULL in Class.

@@ +1674,5 @@
> +BinaryStruct::obj_lookupElement(JSContext *cx, HandleObject obj, uint32_t index,
> +                                MutableHandleObject objp,
> +                                MutableHandleShape propp)
> +{
> +	JS_ASSERT(0);

As before, set to NULL in Class. (Also for subsequent accessors that just assert(0))

@@ +1727,5 @@
> +                             HandleObject receiver, HandleId id,
> +                             MutableHandleValue vp)
> +{
> +    if (!IsBinaryStruct(obj))
> +        return false;

Report an exception.

@@ +1731,5 @@
> +        return false;
> +
> +    RootedObject type(cx, GetType(obj));
> +    if (!IsStructType(type))
> +        return false;

Report an exception.

@@ +1769,5 @@
> +BinaryStruct::obj_getElement(JSContext *cx, HandleObject obj,
> +                             HandleObject receiver, uint32_t index,
> +                             MutableHandleValue vp)
> +{
> +    JS_ASSERT(0);

I'm not 100% certain, but I think that it is perfectly plausible for this function to get called. I believe the correct thing to do, if you do not want to have special treatment for int32_t keys, is to leave the corresponding element in the class structure as NULL. I am trying to verify this.

@@ +1777,5 @@
> +BinaryStruct::obj_getElementIfPresent(JSContext *cx, HandleObject obj,
> +                                      HandleObject receiver, uint32_t index,
> +                                      MutableHandleValue vp, bool *present)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1786,5 @@
> +BinaryStruct::obj_getSpecial(JSContext *cx, HandleObject obj,
> +                             HandleObject receiver, HandleSpecialId sid,
> +                             MutableHandleValue vp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1795,5 @@
> +BinaryStruct::obj_setGeneric(JSContext *cx, HandleObject obj, HandleId id,
> +                             MutableHandleValue vp, JSBool strict)
> +{
> +    if (!IsBinaryStruct(obj))
> +        return false;

Report an exception.

@@ +1799,5 @@
> +        return false;
> +
> +    RootedObject type(cx, GetType(obj));
> +    if (!IsStructType(type))
> +        return false;

Report an exception.

@@ +1808,5 @@
> +    JS_ASSERT(fieldMap);
> +
> +    FieldMap::Ptr fieldInfo;
> +    if (!(fieldInfo = fieldMap->lookup(key)))
> +        return false; // TODO SPEC FIX: are extra properties allowed?

Report an exception.

@@ +1816,5 @@
> +    RootedObject fieldType(cx, fieldInfo->value.type);
> +    if (!ConvertAndCopyTo(cx, fieldType, vp, loc))
> +        return false;
> +
> +    vp.setUndefined();

I think you should not modify `vp` here. The other code I examined did not do so. My guess is that this will affect the result of a `a = b` expression and cause it to be undefined.

@@ +1833,5 @@
> +JSBool
> +BinaryStruct::obj_setElement(JSContext *cx, HandleObject obj, uint32_t index,
> +                             MutableHandleValue vp, JSBool strict)
> +{
> +    JS_ASSERT(0);

As above, supply NULL instead.

@@ +1841,5 @@
> +BinaryStruct::obj_setSpecial(JSContext *cx, HandleObject obj,
> +                             HandleSpecialId sid, MutableHandleValue vp,
> +                             JSBool strict)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1849,5 @@
> +JSBool
> +BinaryStruct::obj_getGenericAttributes(JSContext *cx, HandleObject obj,
> +                                       HandleId id, unsigned *attrsp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1857,5 @@
> +BinaryStruct::obj_getPropertyAttributes(JSContext *cx, HandleObject obj,
> +                                        HandlePropertyName name,
> +                                        unsigned *attrsp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1864,5 @@
> +JSBool
> +BinaryStruct::obj_getElementAttributes(JSContext *cx, HandleObject obj,
> +                                       uint32_t index, unsigned *attrsp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1871,5 @@
> +JSBool
> +BinaryStruct::obj_getSpecialAttributes(JSContext *cx, HandleObject obj,
> +                                       HandleSpecialId sid, unsigned *attrsp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1879,5 @@
> +JSBool
> +BinaryStruct::obj_setGenericAttributes(JSContext *cx, HandleObject obj,
> +                                       HandleId id, unsigned *attrsp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1887,5 @@
> +BinaryStruct::obj_setPropertyAttributes(JSContext *cx, HandleObject obj,
> +                                        HandlePropertyName name,
> +                                        unsigned *attrsp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1894,5 @@
> +JSBool
> +BinaryStruct::obj_setElementAttributes(JSContext *cx, HandleObject obj,
> +                                       uint32_t index, unsigned *attrsp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1901,5 @@
> +JSBool
> +BinaryStruct::obj_setSpecialAttributes(JSContext *cx, HandleObject obj,
> +                                       HandleSpecialId sid, unsigned *attrsp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1909,5 @@
> +JSBool
> +BinaryStruct::obj_deleteProperty(JSContext *cx, HandleObject obj,
> +                                 HandlePropertyName name, JSBool *succeeded)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

@@ +1918,5 @@
> +BinaryStruct::obj_deleteElement(JSContext *cx, HandleObject obj,
> +                                uint32_t index, JSBool *succeeded)
> +{
> +	JS_ASSERT(0);
> +	*succeeded = false;

As above, supply NULL instead.

@@ +1926,5 @@
> +BinaryStruct::obj_deleteSpecial(JSContext *cx, HandleObject obj,
> +                                HandleSpecialId sid, JSBool *succeeded)
> +{
> +	JS_ASSERT(0);
> +	*succeeded = false;

As above, supply NULL instead.

@@ +1934,5 @@
> +JSBool
> +BinaryStruct::obj_enumerate(JSContext *cx, HandleObject obj, JSIterateOp enum_op,
> +                            MutableHandleValue statep, MutableHandleId idp)
> +{
> +	JS_ASSERT(0);

As above, supply NULL instead.

Comment 45

[Niko Matsakis [:nmatsakis]]

Comment on attachment 769189 [details] [diff] [review]
Patch 3: ArrayType and BinaryArray implementation

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

::: js/src/jsbinarydata.cpp
@@ +162,5 @@
> +        return IsSame(cx, elementType1, elementType2);
> +    } else if (IsStructType(type1) && IsStructType(type2)) {
> +        // according to the spec, don't check individual fields.
> +        // they both need to be the *same* struct type
> +        return type1 == type2;

As we said on the phone, this is not correct, we should be checking that the two types are structurally equivalent (that is, not the same object, but representing the same structs).

@@ +542,5 @@
> +    RootedObject obj(cx, NewBuiltinClassInstance(cx, &ArrayType::class_));
> +    if (!obj)
> +        return NULL;
> +
> +    if (!JS_DefineProperty(cx, obj, "elementType", ObjectValue(*elementType),

I expect you want `cx->names().elementType` here, for consistency?

@@ +546,5 @@
> +    if (!JS_DefineProperty(cx, obj, "elementType", ObjectValue(*elementType),
> +                           NULL, NULL, JSPROP_READONLY | JSPROP_PERMANENT))
> +        return NULL;
> +
> +    if (!JS_DefineProperty(cx, obj, "length",

cx->names().length

@@ +552,5 @@
> +                           JSPROP_READONLY | JSPROP_PERMANENT))
> +        return NULL;
> +
> +    Value elementTypeBytes;
> +    if (!JS_GetProperty(cx, elementType, "bytes", &elementTypeBytes))

cx->names().bytes

@@ +561,5 @@
> +    /* since this is the JS visible size and maybe not
> +     * the actual size in terms of memory layout, it is
> +     * always elementType.bytes * length */
> +    if (!JS_DefineProperty(cx, obj, "bytes",
> +                           NumberValue(elementTypeBytes.toInt32() * length),

We need to check for overflow here. I was talking to dherman and there is some question as to what the correct behavior here is.  It is likely we will need to be able to cope with lengths bigger than an int32, although allocating an *instance* of such a type will always OOM.

@@ +566,5 @@
> +                           NULL, NULL, JSPROP_READONLY | JSPROP_PERMANENT))
> +        return NULL;
> +
> +    obj->setFixedSlot(SLOT_MEMSIZE,
> +                      Int32Value(::GetMemSize(cx, elementType) * length));

We also need to be concerned about overflow here.

@@ +661,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    RootedObject thisObj(cx, args.thisv().toObjectOrNull());
> +    if (!IsArrayType(thisObj))
> +        return false;

I think we should throw an exception here?

@@ +708,5 @@
> +
> +    int32_t memsize = GetMemSize(cx, type);
> +    void *memory = JS_malloc(cx, memsize);
> +    memset(memory, 0, memsize);
> +    if (!memory)

probably want to check for NULL before you call memset, not afterwards.

@@ +1221,5 @@
> +        case constant_:\
> +            return NumericType<type_##_t>::reify(cx,\
> +                    ((uint8_t *) owner->getPrivate()) + offset, to);
> +
> +    switch(type->getFixedSlot(0).toInt32()) {

Surely this should have a constant, not getFixedSlot(0)

Comment 46

[Niko Matsakis [:nmatsakis]]

Comment on attachment 769188 [details] [diff] [review]
Patch 2: numeric binary data implementation

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

::: js/src/jsbinarydata.cpp
@@ +164,1 @@
>      return false;

Need an exception here.

::: js/src/jsbinarydata.h
@@ +83,5 @@
> +template <typename T>
> +JS_ALWAYS_INLINE
> +bool NumericType<T>::reify(JSContext *cx, void *mem, MutableHandleValue vp)
> +{
> +    vp.setInt32(* ((T*)mem) );

It'd be simpler, though marginally less efficient, to just have one implementation for all types and call setNumber().

Comment 47

[Niko Matsakis [:nmatsakis]]

Nikhil- The latest version of the patches look much better. I am talking to dherman to clarify a few points (like what to do if the size of a struct exceeds an int32). I marked the smaller patches as r+ but I left the larger ones r- since I'd like to read them over again (it's as much for me as anything else).

One thing we have to do before landing is to ensure that this code is not included in release builds. The correct thing is probably to follow how this is done for ParallelArrays. I'll investigate this and add a comment with a pointer into the code.

Once that is done, I think the best thing is to land this C++ code (which by the way is quite readable and clear, nice job) and then we can proceed to iterate and improve it in various ways:

- Spec additions. We will need to add support for Cursor (or Pointer, or whatever they wind up being called) objects, but that seems fairly straightforward on top of what you have. 

- Minimize allocations. Right now every binary data object requires an object and a JS_malloc allocation. It'd be nice to inline the data into the object itself. This doesn't seem too hard to me, actually, but it's reasonable to do it as a separate patch I think.

- Self-hosting where possible. I still think we will want to transition a lot of this code to be self-hosted, but I am not sure exactly where to draw the line. At minimum, I think we should be able to self-host the ConvertAndCopyTo, ToString, and a bunch of other hairy routines. It is likely that the specialized getElement implementations (and so forth) will remain in C++ code and just be specially integrated into ion.

- TI and jit integration. Shu and I were discussing the plan for integrating into Type Inference and into the jit, and it may involve some changes, but again I think it makes sense to have this code landed before we start experimenting. Basically the idea will be to generalized specialized (and canonical) type objects for the type descriptors. This means that type equality will be reduced to a pointer comparison and also makes the information that the jit needs readily accessible so that we can compile optimize get element calls.

(Note: not saying that all of these bullet points will be implemented by you necessarily, just wanted to lay out the list of things I think are left to do)

Comment 48

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 2: numeric binary data implementation

Carrying forward r=nmatsakis.
Added TypeError to NumericType::cast

Generalizing vp.setNumber() to floats causes ambiguity errors with GCC, dropped that change.

Comment 49

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 3: ArrayType and BinaryArray implementation

Updated as per comments. Fails on struct type equality comparison (logic in structtype patch).

Does not handle the overflow bit at this point.

Comment 50

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 4: StructType and BinaryStruct implementation

Switched to vector for fields, cloned "fields" and address other comments.

Comment 51

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 5: Implement the utility methods in the spec

Rebased. Carrying forward r=nmatsakis.

Comment 52

[Niko Matsakis [:nmatsakis]]

One meta-comment: Eventually I think we will want to have a canonicalized and internal representation of the types, rather than crawling the JS objects themselves (though I believe we only ever reference fixed, immutable fields, so I think the current code is safe). However, I plan to implement this as part of TI/JIT integration, so it doesn't make sense to do much work in that direction now.

Basic plan that Shu and I came up with is to create canonical type objects that represent each new binary data type descriptor, along with an associated canonical type object for handles to that descriptor (aka cursors, pointers) and one for instances of that type descriptor. This gives the jit the full information needed to optimize accesses to binary data objects; of course we'll also want to integrate into the ICs.

Comment 53

[Niko Matsakis [:nmatsakis]]

Also, based on discussion with Dave, regarding overflow, what we want to do is to be consistent with the ES6 treatment of typed arrays:

(1) When creating the type descriptor, we report an error if the length of an array is either fractional or negative. Note that inf and nan are actually considered ok.

(2) When allocating an instance of a type descriptor, we throw an OOM error if the size of that descriptor exceeds uint32 or is inf or nan or otherwise nonsense.

Comment 54

[Niko Matsakis [:nmatsakis]]

Comment on attachment 772858 [details] [diff] [review]
Patch 5: Implement the utility methods in the spec

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

::: js/src/jsbinarydata.cpp
@@ +1017,5 @@
> + * TypedArray would be negative, it is clamped to zero.
> + * see: http://www.khronos.org/registry/typedarray/specs/latest/#7
> + *
> + * TODO: should this duplicate elements or should underlying buffer be shared?
> + * For now it's duplicated

I think the intention is for subarray() to alias.

Comment 55

[Niko Matsakis [:nmatsakis]]

Comment on attachment 772858 [details] [diff] [review]
Patch 5: Implement the utility methods in the spec

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

::: js/src/jsbinarydata.cpp
@@ +789,5 @@
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (args.length() < 1)
> +        return false;

Report an error.

@@ +793,5 @@
> +        return false;
> +
> +    RootedObject thisObj(cx, args.thisv().toObjectOrNull());
> +    if (!IsBlock(thisObj))
> +        return false;

Report an error.

@@ +799,5 @@
> +    RootedValue val(cx, args[0]);
> +    uint8_t *memory = (uint8_t*) thisObj->getPrivate();
> +    RootedObject type(cx, GetType(thisObj));
> +    if (!ConvertAndCopyTo(cx, type, val, memory)) {
> +        ReportTypeError(cx, val, "ArrayType"); // TODO use toString

Either make an issue or remote the TODO.

@@ +813,5 @@
>  {
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (args.length() < 1)
> +        return false;

Report an error.

@@ +1024,4 @@
>  {
>      JS_ASSERT(0);
>      return false;
> +    /*

Why is this commented out?

@@ +1029,3 @@
>  
> +    if (args.length() < 1)
> +        return false; // TODO error about arguments?

Report an error.

@@ +1030,5 @@
> +    if (args.length() < 1)
> +        return false; // TODO error about arguments?
> +
> +    if (!args[0].isInt32())
> +        return false;

Report an error.

@@ +1034,5 @@
> +        return false;
> +
> +    RootedObject thisObj(cx, &args.thisv().toObject());
> +    if (!IsBinaryArray(thisObj))
> +        return false; // TODO error

Report an error.

@@ +1045,5 @@
> +    int32_t end = length;
> +
> +    if (args.length() >= 2) {
> +        if (!args[1].isInt32())
> +            return false;

Report an error.

@@ +1111,5 @@
> +    RootedObject elementType(cx, ArrayType::elementType(cx, type));
> +    for (uint32_t i = 0; i < ArrayType::length(cx, type); i++) {
> +        uint32_t offset = GetMemSize(cx, elementType) * i;
> +        if (!ConvertAndCopyTo(cx, elementType, val, ((uint8_t*) thisObj->getPrivate()) + offset)) {
> +            ReportTypeError(cx, args[0], "Binary data"); // TODO use toString and point out exact elementType

Coding standards forbid TODOs without issue #s. If this seems important enough, we should create follow-up issues.

::: js/src/tests/ecma_6/BinaryData/arraytype.js
@@ +184,5 @@
> +assertEq(rangeNeg[1], 2);
> +
> +assertEq(ma.subarray(-2, -3).length, 0);
> +assertEq(ma.subarray(-6).length, ma.length);
> +*/

Why is this commented out?

Comment 56

[Niko Matsakis [:nmatsakis]]

Comment on attachment 772853 [details] [diff] [review]
Patch 4: StructType and BinaryStruct implementation

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

This looks good. Marking r+ subject to nits below.

::: js/src/jsbinarydata.cpp
@@ +1327,5 @@
> +    if (!GetPropertyNames(cx, fields, JSITER_OWNONLY, &fieldProps))
> +        return false;
> +
> +    // TODO should we really keep this duplicate?
> +    // makes code clearer and simpler

I think you should.

@@ +1506,5 @@
> +        args.rval().setObject(*obj);
> +        return true;
> +    }
> +
> +    //TODO error message

Yes, needs an error message.

@@ +1531,5 @@
> +    StringBuffer contents(cx);
> +    contents.append("StructType({");
> +
> +    Value fieldsVal;
> +    if (!JS_GetProperty(cx, thisObj, "fields", &fieldsVal))

Wouldn't it be easier to iterate over the native FieldList?

@@ +1721,5 @@
> +        return false;
> +    }
> +
> +    RootedObject type(cx, GetType(obj));
> +    if (!IsStructType(type))

Given that IsBinaryStruct is true, shouldn't this be JS_ASSERT(IsStructType(type))

@@ +1729,5 @@
> +    JS_ASSERT(fieldList);
> +
> +    FieldInfo fieldInfo;
> +    if (!LookupFieldList(fieldList, id, &fieldInfo))
> +        return false; // TODO SPEC FIX: are extra properties allowed?

Extra properties are not allowed. Report a suitable error here.

@@ +1792,2 @@
>          } else {
> +            abort();

Change to MOZ_ASSUME_UNREACHABLE("reason")

Comment 57

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: grep -C 3 -i getproperty jsbinarydata.cpp

Comment 58

[Niko Matsakis [:nmatsakis]]

Comment on attachment 772304 [details] [diff] [review]
Patch 2: numeric binary data implementation

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

Subject to the comments below, looks good.

::: js/src/jsbinarydata.cpp
@@ +127,5 @@
> +    }
> +
> +    if (val.isNumber()) {
> +        // NOTE is this the right way to do it?
> +        // Clarify semantics.

Conclusion is that we should coerce, just as typed arrays do. Which implies that this case is never an error, I believe. It also eliminates the distinction between `convert` and `cast`

@@ +210,5 @@
> +
> +    RootedValue arg(cx, args[0]);
> +    T answer;
> +    if (!cast(cx, arg, &answer))
> +    {

NIT: Braces are not necessary here, move comment onto same line as the return.

@@ +349,5 @@
>          return NULL;
>  
>      // Set complexObject.prototype.prototype.__proto__ = Data.prototype
> +    // TODO does this have to actually be a Class so we can set accessor
> +    // properties etc?

I don't really understand what this TODO is asking.

@@ +351,5 @@
>      // Set complexObject.prototype.prototype.__proto__ = Data.prototype
> +    // TODO does this have to actually be a Class so we can set accessor
> +    // properties etc?
> +    RootedObject prototypePrototypeObj(cx, JS_NewObject(cx, NULL, NULL,
> +                global)); if (!LinkConstructorAndPrototype(cx, prototypeObj,

This formatting is odd.

Comment 59

[Niko Matsakis [:nmatsakis]]

Comment on attachment 772849 [details] [diff] [review]
Patch 3: ArrayType and BinaryArray implementation

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

Looks good, subject to comments below.

::: js/src/jsbinarydata.cpp
@@ +654,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +
> +    if (argc != 2 ||
> +        !args[0].isObject() ||
> +        !args[1].isInt32() ||

First, the full spec also provides for array types without specified lengths. But I think we should perhaps leave those for a later patch (I will not this on the bug as a todo item).

Second, users are not required to supply an int32 here. In fact, any double is acceptable so long as it (1) has no fractional part and (2) is not negative.

@@ +655,5 @@
> +
> +    if (argc != 2 ||
> +        !args[0].isObject() ||
> +        !args[1].isInt32() ||
> +        args[1].toInt32() <= 0) {

Nit: I think the coding standard calls for a newline before the `{`

Comment 60

[Niko Matsakis [:nmatsakis]]

Some uncompleted things for which we should make followup bugs:

- buffer property
- handles
- array types with indefinite lengths
- probably more

I'll try to create follow-up bugs soon.

Comment 61

[Niko Matsakis [:nmatsakis]]

Just realized that we should rename jsbinarydata.{cpp,h} to something like builtin/BinaryData.{cpp,h}

Comment 62

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1: Layout the type heirarchy

Moves jsbinarydata.{h,cpp} to builtin/BinaryData.{h,cpp}

Add flags to only enable in Nightly builds.

Comment 63

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Niko Matsakis [:nmatsakis] from comment #58)
> Comment on attachment 772304 [details] [diff] [review]
> Patch 2: numeric binary data implementation
> 
> Review of attachment 772304 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Subject to the comments below, looks good.
> 

> 
> @@ +349,5 @@
> >          return NULL;
> >  
> >      // Set complexObject.prototype.prototype.__proto__ = Data.prototype
> > +    // TODO does this have to actually be a Class so we can set accessor
> > +    // properties etc?
> 
> I don't really understand what this TODO is asking.

What it meant was whether prototype's of the complex classes should have their own Class and overridden JSOps so that they can implement certain behaviour. But that is not required right now. Removed.

Comment 64

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 2: numeric binary data implementation

Changes in convert() to use C like casts.
NumericType<T>::cast() has been reduced to string conversion and coercion.

Comment 65

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 3: ArrayType and BinaryArray implementation

Carrying forward r=nmatsakis with minor fixes as requested.

ObjectClass -> JSObject::class_
Removed/fixed TODOs

Comment 66

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 4: StructType and BinaryStruct implementation

r=nmatsakis with fixes from comment 56 and cleanup of TODOs

Comment 67

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 5: Implement the utility methods in the spec

Fixes from comment 55.

Niko, the subarray bit was commented out because I didn't know whether to alias or duplicate. It's been uncommented, and the bit that will need proper review.

Comment 68

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1: Layout the type heirarchy

Comment 69

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 1 interdiff

Remove duplicate declaration of js_InitBinaryDataClasses.

Move list of binary data in jsapi to standard_class_names, which is where it is supposed to be to avoid the duplicate calls to js_InitBinaryDataClasses [1]

standard_class_atoms seems to be a table for interned global constants. I'm not sure if `Type` should have an entry here, considering it is the 'top-level' of the complex types.

[1] https://tbpl.mozilla.org/?tree=Try&rev=f817b78187ea

Comment 70

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 2: Interdiff

Minor update to apply over Patch 1

Comment 71

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 3: Interdiff

Minor change over existing Patch 3 to rename IsSame to IsSameBinaryDataType, since IsSame is already defined in TypeTraits leading to errors on Try.

Please review attachment 776619 [details] [diff] [review].

Comment 72

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 4: Interdiff

Rename to IsSameBinaryDataType.

Please review attachment 776621 [details] [diff] [review].

Comment 73

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 5: Interdiff

IsSameBinaryDataType.

Please review attachment 776625 [details] [diff] [review]

Comment 74

[Niko Matsakis [:nmatsakis]]

Comment on attachment 776616 [details] [diff] [review]
Patch 2: numeric binary data implementation

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

This isn't quite right, we don't need to distinguish convert from cast anymore.

::: js/src/builtin/BinaryData.cpp
@@ +135,5 @@
> +            if (mozilla::IsInfinite(num) || mozilla::IsNaN(num)) {
> +                *converted = 0;
> +            } else {
> +                *converted = T(num);
> +            }

Can we share this code with typed arrays? That'd be ideal. I haven't looked to see how easy it would be to refactor the relevant TA code into a helper.

@@ +166,5 @@
> +        if (!mozilla::IsNaN(d)) {
> +            // [[CCast]]
> +            *casted = (T) (d);
> +            return true;
> +        }

Actually, I think we should be doing this on the convert path as well. The idea is that 

    arr[0] = "1"

should work the same regardless of whether `arr = new Int32Array(...)` or `new ArrayType(int32, ...)`.

I don't believe there is any need to distinguish convert from cast any longer.

Comment 75

[Niko Matsakis [:nmatsakis]]

Comment on attachment 776616 [details] [diff] [review]
Patch 2: numeric binary data implementation

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

This isn't quite right, we don't need to distinguish convert from cast anymore.

(Changing to r+, since I'm happy with this as long as convert/cast are merged.)

::: js/src/builtin/BinaryData.cpp
@@ +135,5 @@
> +            if (mozilla::IsInfinite(num) || mozilla::IsNaN(num)) {
> +                *converted = 0;
> +            } else {
> +                *converted = T(num);
> +            }

Can we share this code with typed arrays? That'd be ideal. I haven't looked to see how easy it would be to refactor the relevant TA code into a helper.

@@ +166,5 @@
> +        if (!mozilla::IsNaN(d)) {
> +            // [[CCast]]
> +            *casted = (T) (d);
> +            return true;
> +        }

Actually, I think we should be doing this on the convert path as well. The idea is that 

    arr[0] = "1"

should work the same regardless of whether `arr = new Int32Array(...)` or `new ArrayType(int32, ...)`.

I don't believe there is any need to distinguish convert from cast any longer.

Comment 76

[Niko Matsakis [:nmatsakis]]

Comment on attachment 776625 [details] [diff] [review]
Patch 5: Implement the utility methods in the spec

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

::: js/src/builtin/BinaryData.cpp
@@ +1080,5 @@
> +    RootedObject typeProto(cx);
> +    if (!JSObject::getProto(cx, type, &typeProto))
> +        return false;
> +
> +    RootedValue arrayTypeGlobalVal(cx);

Maybe easier and clearer to add something like getOrCreateDataObject to the Global object?

Comment 77

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 2: numeric binary data implementation

Removed cast, unified parts of BD NumericType::convert() and TA obj_setElementTail().

Comment 78

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Patch 5: Implement the utility methods in the spec

Add a JSProto key for ArrayTypeObject which is the ArrayType() constructor and getOrCreateArrayTypeObject(), so subarray() can pluck it off the global.

Comment 79

[Niko Matsakis [:nmatsakis]]

Comment on attachment 777252 [details] [diff] [review]
Patch 1: Layout the type heirarchy

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

::: js/src/builtin/BinaryData.cpp
@@ +25,5 @@
> +{
> +    return ReportIsNotFunction(cx, *vp);
> +}
> +
> +// FIXME will actually require knowing function name

What does this FIXME mean?

@@ +148,5 @@
> +static JSObject *
> +InitComplexClasses(JSContext *cx, Handle<GlobalObject *> global)
> +{
> +    // TODO FIXME use DefineConstructorAndPrototype and other
> +    // utilities

It's not clear to me what you would change here.

Comment 80

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Niko Matsakis [:nmatsakis] from comment #79)
> Comment on attachment 777252 [details] [diff] [review]
> Patch 1: Layout the type heirarchy
> 
> Review of attachment 777252 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/BinaryData.cpp
> @@ +25,5 @@
> > +{
> > +    return ReportIsNotFunction(cx, *vp);
> > +}
> > +
> > +// FIXME will actually require knowing function name
> 
> What does this FIXME mean?
> 
> @@ +148,5 @@
> > +static JSObject *
> > +InitComplexClasses(JSContext *cx, Handle<GlobalObject *> global)
> > +{
> > +    // TODO FIXME use DefineConstructorAndPrototype and other
> > +    // utilities
> 
> It's not clear to me what you would change here.

Both of these have been fixed in subsequent patches.

Comment 81

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

https://tbpl.mozilla.org/?tree=Try&rev=f7769dbb38d8

Comment 82

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5dd7854c9474
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2aa4435cd798
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8253f5b39cbd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/43d1eada77d6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/13b28328f010

Comment 83

[Nicholas Nethercote [inactive]]

The modelines at the top of the new files aren't right.  They should look like this:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
 * vim: set ts=8 sts=4 et sw=4 tw=99:
 * This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Comment 84

[Niko Matsakis [:nmatsakis]]

I marked this bug as [leave open] because I propose to continue using it as a meta bug to track the remaining features etc. I am creating various bugs that block this one.

Comment 85

[Niko Matsakis [:nmatsakis]]

Note: I addressed the incorrect header comments pointed out by njn in bug 898338.

Comment 86

[Niko Matsakis [:nmatsakis]]

Some other things that I noticed while playing around with the code:

1. PixelType({x: 1, y: 2}) does not work
2. obj_enumerate must be implemented for struct types (bug 898371) 
3. I don't believe the jsids for struct types are being properly rooted

Point 1 is minor. Points 2 and 3 are things that I should have realized while reviewing, I apologize. Point 2 makes me wonder if there are other hooks that should be implemented; when I last investigated, I convinced myself that they were largely optional, but I want to re-read the code again to be sure. It might be that the best thing is to pull the patches and re-push after correcting these errors.

Comment 87

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5dd7854c9474
https://hg.mozilla.org/mozilla-central/rev/2aa4435cd798
https://hg.mozilla.org/mozilla-central/rev/8253f5b39cbd
https://hg.mozilla.org/mozilla-central/rev/43d1eada77d6
https://hg.mozilla.org/mozilla-central/rev/13b28328f010

Comment 88

[David Bruant]

I have the proper keyboard for this :-p
"née" is feminine. "nés" is plural masculine (because there are several typed objects and "objects" translates as "objets" which is masculine in French... I think every foreign word is masculine in French anyway)

Comment 89

[Sylvestre Ledru [:Sylvestre]]

Seems that we can close it now