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

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

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

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

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/

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.

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.

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.

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

Un-bitrotted the patches.

Update to work with latest Spidermonkey, especially rooting changes.

Comment on attachment 752284 [details] [diff] [review] Part 1 - Layout the type heirarchy Adopting review (with permission).

Comment on attachment 752286 [details] [diff] [review] Part 2 - numeric binary data implementation Adopting review (with permission).

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 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 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 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 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.

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.

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

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

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

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 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.

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

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 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 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().

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)

Carrying forward r=nmatsakis. Added TypeError to NumericType::cast Generalizing vp.setNumber() to floats causes ambiguity errors with GCC, dropped that change.

Updated as per comments. Fails on struct type equality comparison (logic in structtype patch). Does not handle the overflow bit at this point.

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

Rebased. Carrying forward r=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.

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 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 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 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 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 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 `{`

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.

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

Moves jsbinarydata.{h,cpp} to builtin/BinaryData.{h,cpp} Add flags to only enable in Nightly builds.

(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.

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

Carrying forward r=nmatsakis with minor fixes as requested. ObjectClass -> JSObject::class_ Removed/fixed TODOs

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

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.

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

Minor update to apply over Patch 1

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].

Rename to IsSameBinaryDataType. Please review attachment 776621 [details] [diff] [review].

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

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 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 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?

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

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

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.

(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.

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

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/. */

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.

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

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.

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

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)