The default bug view has changed. See this FAQ.
Bug 578700 (harmony:typedobjects)

[meta] Harmony typed objects (nés binary data)

NEW
Unassigned

Status

()

Core
JavaScript Engine
7 years ago
2 years ago

People

(Reporter: dherman, Unassigned)

Tracking

(Depends on: 16 bugs, Blocks: 2 bugs, {dev-doc-needed, feature})

unspecified
dev-doc-needed, feature
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open], URL)

Attachments

(9 attachments, 35 obsolete attachments)

6.37 KB, text/plain
Details
53.86 KB, patch
nsm
: review+
nsm
: checkin+
Details | Diff | Splinter Review
36.22 KB, patch
nsm
: review+
nsm
: checkin+
Details | Diff | Splinter Review
20.25 KB, patch
nmatsakis
: review+
nsm
: checkin+
Details | Diff | Splinter Review
5.45 KB, patch
Details | Diff | Splinter Review
6.25 KB, patch
Details | Diff | Splinter Review
7.69 KB, patch
Details | Diff | Splinter Review
30.15 KB, patch
nmatsakis
: review+
nsm
: checkin+
Details | Diff | Splinter Review
24.81 KB, patch
nmatsakis
: review+
nsm
: checkin+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
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/
Assignee: general → nsm.nikhil
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Blocks: 694100
I believe Nikhil doesn't have time for this at the moment, so clearing assignee.
Assignee: nsm.nikhil → general
Status: ASSIGNED → NEW
Assignee: general → nsm.nikhil

Updated

5 years ago
Alias: harmony:bindata
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/
Created attachment 676376 [details] [diff] [review]
Part 1 - Layout the type heirarchy
Created attachment 676377 [details] [diff] [review]
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.
Attachment #676377 - Flags: feedback?(jorendorff)
Created attachment 676378 [details] [diff] [review]
Part 3 - ArrayType and BinaryArray implementation
Created attachment 676380 [details] [diff] [review]
Part 4 - StructType and BinaryStruct implementation
Created attachment 676382 [details] [diff] [review]
Part 5 - Implement the utility methods in the spec
Created attachment 676385 [details] [diff] [review]
Part 6 - Memory management and GC related fixes
Attachment #676376 - Flags: review?(luke)
Attachment #676378 - Flags: review?(luke)
Attachment #676380 - Flags: review?(luke)
Attachment #676382 - Flags: review?(luke)
Attachment #676385 - Flags: review?(luke)

Comment 11

4 years ago
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

4 years ago
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.
Keywords: dev-doc-needed
Attachment #676376 - Flags: review?(luke) → review?(wmccloskey)
Attachment #676377 - Flags: feedback?(jorendorff) → review?(wmccloskey)
Attachment #676378 - Flags: review?(luke) → review?(wmccloskey)
Attachment #676380 - Flags: review?(luke) → review?(wmccloskey)
Attachment #676382 - Flags: review?(luke) → review?(wmccloskey)
Attachment #676385 - Flags: review?(luke) → review?(wmccloskey)
See also bug 749786 -- int64/uint64 value types. Happy to track this bug to reuse template-ology.

/be
Created attachment 736349 [details] [diff] [review]
Part 1 - Layout the type heirarchy

Un-bitrotted the patches.
Attachment #676376 - Attachment is obsolete: true
Attachment #676376 - Flags: review?(wmccloskey)
Created attachment 736351 [details] [diff] [review]
Part 2 - numeric binary data implementation
Attachment #676377 - Attachment is obsolete: true
Attachment #676377 - Flags: review?(wmccloskey)
Created attachment 736352 [details] [diff] [review]
Part 3 - ArrayType and BinaryArray implementation
Attachment #676378 - Attachment is obsolete: true
Attachment #676378 - Flags: review?(wmccloskey)
Created attachment 736353 [details] [diff] [review]
Part 4 - StructType and BinaryStruct implementation
Attachment #676380 - Attachment is obsolete: true
Attachment #676380 - Flags: review?(wmccloskey)
Created attachment 736354 [details] [diff] [review]
Part 5 - Implement the utility methods in the spec
Attachment #676382 - Attachment is obsolete: true
Attachment #676382 - Flags: review?(wmccloskey)
Created attachment 736356 [details] [diff] [review]
Part 6 - Memory management and GC related fixes
Attachment #676385 - Attachment is obsolete: true
Attachment #676385 - Flags: review?(wmccloskey)
Created attachment 752284 [details] [diff] [review]
Part 1 - Layout the type heirarchy

Update to work with latest Spidermonkey, especially rooting changes.
Attachment #736349 - Attachment is obsolete: true
Attachment #752284 - Flags: review?(luke)
Created attachment 752285 [details] [diff] [review]
Part 2 - numeric binary data implementation
Attachment #752285 - Flags: review?(luke)
Created attachment 752286 [details] [diff] [review]
Part 2 - numeric binary data implementation
Attachment #736351 - Attachment is obsolete: true
Attachment #752285 - Attachment is obsolete: true
Attachment #752285 - Flags: review?(luke)
Attachment #752286 - Flags: review?(luke)
Created attachment 752287 [details] [diff] [review]
Part 3 - ArrayType and BinaryArray implementation
Attachment #736352 - Attachment is obsolete: true
Attachment #752287 - Flags: review?(luke)
Created attachment 752288 [details] [diff] [review]
Part 4 - StructType and BinaryStruct implementation
Attachment #736353 - Attachment is obsolete: true
Attachment #752288 - Flags: review?(luke)
Created attachment 752291 [details] [diff] [review]
Part 5 - Implement the utility methods in the spec
Attachment #736354 - Attachment is obsolete: true
Attachment #752291 - Flags: review?(luke)
Created attachment 752294 [details] [diff] [review]
Part 6 - Memory management and GC related fixes
Attachment #736356 - Attachment is obsolete: true
Attachment #752294 - Flags: review?(luke)
Comment on attachment 752284 [details] [diff] [review]
Part 1 - Layout the type heirarchy

Adopting review (with permission).
Attachment #752284 - Flags: review?(luke) → review?(nmatsakis)
Comment on attachment 752286 [details] [diff] [review]
Part 2 - numeric binary data implementation

Adopting review (with permission).
Attachment #752286 - Flags: review?(luke) → review?(nmatsakis)

Updated

4 years ago
Attachment #752287 - Flags: review?(luke) → review?(nmatsakis)

Updated

4 years ago
Attachment #752288 - Flags: review?(luke) → review?(nmatsakis)

Updated

4 years ago
Attachment #752291 - Flags: review?(luke) → review?(nmatsakis)

Updated

4 years ago
Attachment #752294 - Flags: review?(luke) → review?(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.
Attachment #752284 - Flags: review?(nmatsakis)
Attachment #752284 - Flags: review-
Attachment #752284 - Flags: feedback+
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?
Attachment #752286 - Flags: review?(nmatsakis)
Attachment #752286 - Flags: review-
Attachment #752286 - Flags: feedback+
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.
Attachment #752287 - Flags: review?(nmatsakis)
Attachment #752287 - Flags: review-
Attachment #752287 - Flags: feedback+
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.
Attachment #752288 - Flags: review?(nmatsakis)
Attachment #752288 - Flags: review-
Attachment #752288 - Flags: feedback+
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.
Attachment #752294 - Flags: review?(nmatsakis)
Attachment #752294 - Flags: review-
Attachment #752294 - Flags: feedback+
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.
Created attachment 757541 [details] [diff] [review]
Patch 1: Layout the type heirarchy

@nmatsakis: Is this along the lines of your comments?
Attachment #752284 - Attachment is obsolete: true
Attachment #757541 - Flags: review?(nmatsakis)
Created attachment 757564 [details] [diff] [review]
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)
Attachment #752286 - Attachment is obsolete: true
Attachment #757564 - Flags: review?(nmatsakis)
Blocks: 801869
:nmatsakis, please hold off from reviewing on Friday. I'll have some significantly cleaned up patches up by the end of Friday. Thanks!
Created attachment 769187 [details] [diff] [review]
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.
Attachment #757541 - Attachment is obsolete: true
Attachment #757541 - Flags: review?(nmatsakis)
Attachment #769187 - Flags: review?(nmatsakis)
Created attachment 769188 [details] [diff] [review]
Patch 2: numeric binary data implementation
Attachment #757564 - Attachment is obsolete: true
Attachment #757564 - Flags: review?(nmatsakis)
Attachment #769188 - Flags: review?(nmatsakis)
Created attachment 769189 [details] [diff] [review]
Patch 3: ArrayType and BinaryArray implementation
Attachment #752287 - Attachment is obsolete: true
Attachment #769189 - Flags: review?(nmatsakis)
Created attachment 769191 [details] [diff] [review]
Patch 4: StructType and BinaryStruct implementation
Attachment #752288 - Attachment is obsolete: true
Attachment #752294 - Attachment is obsolete: true
Attachment #769191 - Flags: review?(nmatsakis)
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.
Attachment #752291 - Attachment is obsolete: true
Attachment #752291 - Flags: review?(nmatsakis)
Created attachment 769214 [details] [diff] [review]
Patch 5: Implement the utility methods in the spec

subarray is commented since the spec has erased it and there are compilation issues.
Attachment #769214 - Flags: review?(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.
Attachment #769191 - Flags: review?(nmatsakis)
Attachment #769191 - Flags: review-
Attachment #769191 - Flags: feedback+
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)
Attachment #769189 - Flags: review?(nmatsakis)
Attachment #769189 - Flags: review-
Attachment #769189 - Flags: feedback+
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().
Attachment #769188 - Flags: review?(nmatsakis) → review+
Attachment #769187 - Flags: review?(nmatsakis) → review+
Attachment #769214 - Flags: review?(nmatsakis) → review+
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)
Created attachment 772304 [details] [diff] [review]
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.
Attachment #769188 - Attachment is obsolete: true
Attachment #772304 - Flags: review+
Created attachment 772849 [details] [diff] [review]
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.
Attachment #769189 - Attachment is obsolete: true
Attachment #772849 - Flags: review?(nmatsakis)
Created attachment 772853 [details] [diff] [review]
Patch 4: StructType and BinaryStruct implementation

Switched to vector for fields, cloned "fields" and address other comments.
Attachment #769214 - Attachment is obsolete: true
Attachment #772853 - Flags: review?(nmatsakis)
Attachment #769191 - Attachment is obsolete: true
Created attachment 772858 [details] [diff] [review]
Patch 5: Implement the utility methods in the spec

Rebased. Carrying forward r=nmatsakis.
Attachment #772858 - Flags: review+
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")
Attachment #772853 - Flags: review?(nmatsakis) → review+
Created attachment 774769 [details]
grep -C 3 -i getproperty jsbinarydata.cpp
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 `{`
Attachment #772849 - Flags: review?(nmatsakis) → review+
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}
Created attachment 776613 [details] [diff] [review]
Patch 1: Layout the type heirarchy

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

Add flags to only enable in Nightly builds.
Attachment #769187 - Attachment is obsolete: true
Attachment #776613 - Flags: review?(nmatsakis)
(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.
Created attachment 776616 [details] [diff] [review]
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.
Attachment #772304 - Attachment is obsolete: true
Attachment #776616 - Flags: review?(nmatsakis)
Created attachment 776619 [details] [diff] [review]
Patch 3: ArrayType and BinaryArray implementation

Carrying forward r=nmatsakis with minor fixes as requested.

ObjectClass -> JSObject::class_
Removed/fixed TODOs
Attachment #772849 - Attachment is obsolete: true
Attachment #776619 - Flags: review+
Created attachment 776621 [details] [diff] [review]
Patch 4: StructType and BinaryStruct implementation

r=nmatsakis with fixes from comment 56 and cleanup of TODOs
Attachment #772853 - Attachment is obsolete: true
Attachment #776621 - Flags: review+
Created attachment 776625 [details] [diff] [review]
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.
Attachment #772858 - Attachment is obsolete: true
Attachment #776625 - Flags: review?(nmatsakis)
Created attachment 777252 [details] [diff] [review]
Patch 1: Layout the type heirarchy
Attachment #776613 - Attachment is obsolete: true
Attachment #776613 - Flags: review?(nmatsakis)
Attachment #777252 - Flags: review?(nmatsakis)
Created attachment 777257 [details] [diff] [review]
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
Created attachment 777261 [details] [diff] [review]
Patch 2: Interdiff

Minor update to apply over Patch 1
Created attachment 777267 [details] [diff] [review]
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].
Created attachment 777269 [details] [diff] [review]
Patch 4: Interdiff

Rename to IsSameBinaryDataType.

Please review attachment 776621 [details] [diff] [review].
Created attachment 777271 [details] [diff] [review]
Patch 5: Interdiff

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.
Attachment #776616 - Flags: review?(nmatsakis)
Attachment #776616 - Flags: review-
Attachment #776616 - Flags: feedback+
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.
Attachment #776616 - Flags: review-
Attachment #776616 - Flags: review+
Attachment #776616 - Flags: feedback+
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?
Attachment #776625 - Flags: review?(nmatsakis) → review+
Created attachment 781071 [details] [diff] [review]
Patch 2: numeric binary data implementation

Removed cast, unified parts of BD NumericType::convert() and TA obj_setElementTail().
Attachment #776616 - Attachment is obsolete: true
Attachment #777261 - Attachment is obsolete: true
Attachment #781071 - Flags: review?(nmatsakis)
Created attachment 781075 [details] [diff] [review]
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.
Attachment #776625 - Attachment is obsolete: true
Attachment #777271 - Attachment is obsolete: true
Attachment #781075 - Flags: review?(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.
Attachment #777252 - Flags: review?(nmatsakis) → review+
Attachment #781071 - Flags: review?(nmatsakis) → review+
Attachment #781075 - Flags: review?(nmatsakis) → review+
(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.
https://tbpl.mozilla.org/?tree=Try&rev=f7769dbb38d8
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/. */
Depends on: 898338
Depends on: 898342
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.
Whiteboard: [leave open]
Depends on: 898347
Depends on: 898349
Depends on: 898350
Summary: Harmony binary data objects → [meta] Harmony binary data objects
Note: I addressed the incorrect header comments pointed out by njn in bug 898338.
Depends on: 898356
Depends on: 898359
Depends on: 898362
Depends on: 898363
Depends on: 898371
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
Flags: in-testsuite+
Depends on: 898494
Depends on: 898623
Depends on: 898628
Depends on: 898630
Depends on: 898644
Depends on: 898622
Depends on: 898649
Depends on: 898664
Depends on: 898661
Depends on: 898670
Depends on: 898671
Depends on: 898675
Depends on: 898695
Depends on: 898734
Attachment #776621 - Flags: checkin+
Attachment #777252 - Flags: checkin+
Attachment #781071 - Flags: checkin+
Attachment #781075 - Flags: checkin+
Attachment #776619 - Flags: checkin+
Depends on: 904348
Depends on: 914220
Alias: harmony:bindata → harmony:typedobjects
Summary: [meta] Harmony binary data objects → [meta] Harmony typed objects (nee binary data)

Comment 88

4 years ago
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)
Summary: [meta] Harmony typed objects (nee binary data) → [meta] Harmony typed objects (nés binary data)
Blocks: 917454
No longer blocks: 917454
Depends on: 917454
Depends on: 922115
Depends on: 922172
Depends on: 922216
Depends on: 925541
Keywords: feature
Depends on: 929651
Depends on: 929656
Depends on: 930634
Depends on: 932464
Depends on: 933269
Depends on: 933277
Depends on: 933289
Depends on: 933293
Depends on: 933295
Depends on: 933758
Depends on: 933760
Depends on: 933762
Blocks: 937391
Depends on: 938728
Blocks: 939715
Blocks: 757804
Depends on: 945808
Depends on: 950106
Depends on: 952114
Assignee: nsm.nikhil → nobody
Depends on: 961821
Depends on: 966575
Depends on: 968866
Depends on: 969578
Depends on: 972398
Depends on: 972400
Depends on: 972403
Depends on: 972417
Depends on: 973237
Depends on: 973238
Depends on: 976688

Updated

3 years ago
Depends on: 978077
Depends on: 983977
Depends on: 983986

Updated

3 years ago
Depends on: 994018
Blocks: 1021376
No longer blocks: 694100
Depends on: 1058340
Depends on: 1092318
Depends on: 1107145
Depends on: 1107226
Depends on: 1128298
Depends on: 1128299
You need to log in before you can comment on or make changes to this bug.