922115 - Support unsized arrays in typed objects

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Current implementation only supports typed object arrays of fixed size. Full spec includes "unsized" arrays.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug922115.diff

Comment 2

[Niko Matsakis [:nmatsakis]]

Try run (green but for 1 failure that looks...very unlikely to be related): https://tbpl.mozilla.org/?tree=Try&rev=14c2971ba368

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8337176 [details] [diff] [review]
Bug922115.diff

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

r+ dependent on an answer to the UnsizedArrayTypeRepresentation::Create question.

::: js/src/builtin/TypeRepresentation.cpp
@@ +755,5 @@
> +        if (!NumberValueToStringBuffer(cx, NumberValue(length()), contents))
> +            return false;
> +
> +        if (arrayType->element()->isSizedArray()) {
> +            if (!contents.append(","))

Funky... so an array of four 3-element arrays of ints would be "int.array(4,3)" instead of "Array(4, Array(3, int))" (or "int[3][4]")?

@@ +923,3 @@
>      memset(mem, 0, size());
>      if (opaque())
>          visitReferences(this, mem, visitor);

You don't want to call

  TypeRepresentation::initInstance(rt, mem);

to do these 3 lines? Maybe that is weird. Just seems a shame to duplicate the code.

::: js/src/builtin/TypeRepresentation.h
@@ +163,5 @@
> +    }
> +
> +    SizedTypeRepresentation *asSized() {
> +        JS_ASSERT(isSized());
> +        return (SizedTypeRepresentation*) this;

Can you use a static_cast<> instead?

@@ +199,5 @@
>      }
>  
> +    SizedArrayTypeRepresentation *asSizedArray() {
> +        JS_ASSERT(isSizedArray());
> +        return (SizedArrayTypeRepresentation*) this;

and here, and the other places too (sorry, I know this is done differently in the existing code, but a C-style cast of 'this' gives me the willies even if I know there aren't any inheritance tricks going on here.)

@@ +223,5 @@
>  
>      void mark(JSTracer *tracer);
>  };
>  
> +class SizedTypeRepresentation : public TypeRepresentation{

need the space back before the '{'

@@ +406,5 @@
> +};
> +
> +class SizedArrayTypeRepresentation : public SizedTypeRepresentation {
> +  private:
> +    // so TypeRepresentation can call appendStringArray() etc

appendStringSizedArray (in the comment)

@@ +488,5 @@
>      const StructField *fieldNamed(jsid id) const;
>  
> +    // Creates a struct type containing fields with names from `ids`
> +    // and types from `typeReprOwners`. The latter should be the owner
> +    // objects of a set of sized type representations.

thanks for the comment

::: js/src/builtin/TypedObject.cpp
@@ +328,5 @@
> +
> +      case TypeRepresentation::UnsizedArray:
> +        return typeRepr->asUnsizedArray()->element()->size() * DatumLength(val);
> +    }
> +    MOZ_ASSUME_UNREACHABLE("unhandle typerepresentation kind");

*unhandled

@@ +346,5 @@
> +{
> +    if (!IsTypedDatum(obj))
> +        return false;
> +    TypeRepresentation *repr = typeRepresentation(*GetType(obj));
> +    return repr->isSizedArray() || repr->isUnsizedArray();

you say this in at least 2 places -- is it worth an isArray()?

@@ +352,5 @@
> +
> +// Extracts the `prototype` property from `obj`, throwing if it is
> +// missing or not an object.
> +static JSObject *
> +Prototype(JSContext *cx, HandleObject obj)

GetPrototype, since this can fail?

@@ +689,3 @@
>      RootedObject obj(
>          cx, NewObjectWithClassProto(cx, &ArrayType::class_,
> +                                    &*arrayTypePrototype, nullptr));

why the &* punctuation? This should just accept arrayTypePrototype directly.

@@ +747,2 @@
>  
> +    // construct the type repr (either unsized or sized)

Sorry, I've been staring at this too long, so I'll just ask. Why is it ok to always use UnsizedArrayTypeRepresentation::Create? I'm not understanding it.

@@ +802,4 @@
>      if (!obj)
>          return false;
> +
> +    // Add `length` property which is only found on unsized arrays.

http://wiki.ecmascript.org/doku.php?id=harmony:typed_objects says the property exists on all arrays, it's just undefined on unsized arrays. I guess that's changed?

@@ +808,5 @@
> +                                  lengthVal, nullptr, nullptr,
> +                                  JSPROP_READONLY | JSPROP_PERMANENT))
> +        return nullptr;
> +
> +    // Add `unsized` property which is only found.

Sounds like this replaces the 'variable' property I see right now in the spec? Need to finish sentence in comment.

@@ +1000,4 @@
>  
>      RootedObject obj(
>          cx, NewObjectWithClassProto(cx, &StructType::class_,
> +                                    &*structTypePrototype, nullptr));

another &*

@@ +1620,5 @@
>      setReservedSlot(JS_DATUM_SLOT_OWNER, ObjectValue(owner));
>  }
>  
> +static int32_t
> +LengthForType(JSObject &type)

Can you give this a more descriptive name? At first glance, it looks totally broken given its current name.

::: js/src/builtin/TypedObject.h
@@ +337,5 @@
>                                       HandleObject type,
>                                       HandleObject typedContents,
>                                       size_t offset);
>  
> +

is the extra blank line intentional?

::: js/src/jit-test/tests/TypedObject/jit-complex.js
@@ +8,5 @@
>                                              y: TypedObject.float64});
>  var LineType = new TypedObject.StructType({source: PointType,
>                                             target: PointType});
>  
>  function manhattenDistance(line) {

"manhattan"

@@ +10,5 @@
>                                             target: PointType});
>  
>  function manhattenDistance(line) {
> +  print(line.target.x,line.source.x,
> +        line.target.y,line.source.y);

do you want to keep the debugging prints?

@@ +25,5 @@
>    var fromAToB = new LineType({source: {x: 22, y: 44},
>                                 target: {x: 66, y: 88}});
>  
> +  for (var i = 0; i < N; i++) {
> +    print(fromAToB.toSource());

debugging prints

::: js/src/jit-test/tests/TypedObject/jit-read-many.js
@@ +50,5 @@
> +    bar(array1, i, i + 1);
> +    bar(array2, i, i + 2);
> +  }
> +
> +  // ...and some more.

Nice test

::: js/src/jit/IonBuilder.cpp
@@ +6643,5 @@
> +    // typed object.  We know it's an int32, so we can convert from
> +    // Value to int32 using truncation.
> +    size_t lenOfAll = objTypeReprs.arrayLength();
> +    MDefinition *length;
> +    if (lenOfAll < size_t(INT_MAX)) {

You're using INT_MAX as a sentinel value, so I'd prefer

  lenOfAll != size_t(INT_MAX)

::: js/src/jit/TypeRepresentationSet.cpp
@@ +244,5 @@
> +      {
> +        const size_t result = get(0)->asSizedArray()->length();
> +        for (size_t i = 1; i < length(); i++) {
> +            if (get(i)->asSizedArray()->length() != result)
> +                return SIZE_MAX;

My initial question on reading this was "why not return the max of all lengths?" Can you comment that you're using SIZE_MAX as a sentinel value to indicate "variable size or not all the same size"? Or use a different name for the SIZE_MAX sentinel, perhaps, since it's confusing for it to be SIZE_MAX here and size_t(INT_MAX) in IonBuilder. NONCONSTANT_SIZE?

::: js/src/js.msg
@@ +430,5 @@
>  MSG_DEF(JSMSG_EXPORT_DECL_AT_TOP_LEVEL, 376, 0, JSEXN_SYNTAXERR, "export declarations may only appear at top level")
>  MSG_DEF(JSMSG_RC_AFTER_EXPORT_SPEC_LIST, 377, 0, JSEXN_SYNTAXERR, "missing '}' after export specifier list")
>  MSG_DEF(JSMSG_NO_EXPORT_NAME,           378, 0, JSEXN_SYNTAXERR, "missing export name")
>  MSG_DEF(JSMSG_DECLARATION_AFTER_EXPORT, 379, 0, JSEXN_SYNTAXERR, "missing declaration after 'export' keyword")
> +MSG_DEF(JSMSG_TYPEDOBJECT_INVALID_PROTOTYPE, 380, 0, JSEXN_TYPEERR, "prototype field is not an object")

I would remove the _TYPEDOBJECT part. Seems generally useful.

::: js/src/tests/ecma_6/TypedObject/unsizedarrays.js
@@ +11,5 @@
> +var StructType = TypedObject.StructType;
> +var uint8 = TypedObject.uint8;
> +var float32 = TypedObject.float32;
> +var uint32 = TypedObject.uint32;
> +var ObjectType = TypedObject.Object;

You no like

  var { ArrayType, StructType, ... } = TypedObject;

?

::: js/src/tests/ecma_6/TypedObject/unsizedarraysembedded.js
@@ +12,5 @@
> +function runTests() {
> +  print(BUGNUMBER + ": " + summary);
> +
> +  var Uints = T.uint32.array();
> +  assertThrowsInstanceOf(function() { new T.StructType({f: Uints}) }, TypeError);

Use () => ... to be consistent with your other tests?

Comment 4

[Niko Matsakis [:nmatsakis]]

Quick response to this question:

> Sorry, I've been staring at this too long, so I'll just ask. Why is it ok to always use
> UnsizedArrayTypeRepresentation::Create? I'm not understanding it.

The answer is that, in the new API, one writes:

    var Es = new ArrayType(E)

to get an unsized array of E elements. You can then fix Es to a specific length using `dimension()`:

    var Es22 = Es.dimension(22);

Or, you can avoid these two steps via the `array()` shorthand:

    var Es22 = E.array(22);

The motivation for this was that every sized array type (`Es22`) has an associated unsized array type (`Es`) from which it was derived. For some operations that can be applied to instances of the sized array type, e.g., `filter()`, the result type will be this associated unsized array type:

    var es22 = new Es22();
    assert(objType(es22) === Es22)
    var es = es22.filter(i => ...);
    assert(objType(es) === Es);

Comment 5

[Steve Fink [:sfink] [:s:]]

Oh, sorry. I hadn't realized that ArrayType::construct *always* creates an unsized array. For some reason, I was thinking that it would optionally take a size param and produce a sized array. Maybe it's because of this comment:

 // construct the type repr (either unsized or sized)

Why is it saying the type repr can be either unsized or sized? It looks like it's always unsized here. Or am I misunderstanding the comment?

Comment 6

[Niko Matsakis [:nmatsakis]]

(In reply to Steve Fink [:sfink] from comment #5)
> Or am I misunderstanding the comment?

The comment is indeed bogus and I have removed it.

Comment 7

[Niko Matsakis [:nmatsakis]]

(In reply to Steve Fink [:sfink] from comment #3)
> Funky... so an array of four 3-element arrays of ints would be
> "int.array(4,3)" instead of "Array(4, Array(3, int))" (or "int[3][4]")?

Yes. This is how it works in C and Java as well (i.e., T[X][Y] is an array of dimension X of arrays of dimension Y of T). The reasoning is that one writes "E.array(X, Y, Z)" to create an array that can be indexed as "e[x][y][z]".

 You don't want to call
> 
>   TypeRepresentation::initInstance(rt, mem);
> 
> to do these 3 lines? Maybe that is weird. Just seems a shame to duplicate
> the code.

Not sure what is being duplicated -- this is the implementation of initInstance?
I do have some recollection of being vaguely dissatisfied with that section of
the code but thinking that the way I did it worked out best overall, but I can't
remember why right now.

> http://wiki.ecmascript.org/doku.php?id=harmony:typed_objects says the
> property exists on all arrays, it's just undefined on unsized arrays. I
> guess that's changed?

Yes, you're right, my mistake.

> Sounds like this replaces the 'variable' property I see right now in the
> spec? Need to finish sentence in comment.

No, this is a link from the sized type to the unsized type. It is not included
in the spec yet.

> You no like
> 
>   var { ArrayType, StructType, ... } = TypedObject;

Neat! I forgot about that.

Comment 8

[Steve Fink [:sfink] [:s:]]

(In reply to Niko Matsakis [:nmatsakis] from comment #7)
> (In reply to Steve Fink [:sfink] from comment #3)
>  You don't want to call
> > 
> >   TypeRepresentation::initInstance(rt, mem);
> > 
> > to do these 3 lines? Maybe that is weird. Just seems a shame to duplicate
> > the code.
> 
> Not sure what is being duplicated -- this is the implementation of
> initInstance? I do have some recollection of being vaguely dissatisfied with that section
> of the code but thinking that the way I did it worked out best overall, but I
> can't remember why right now.

This is the implementation of SizedTypeRepresentation::initInstance, and the very first part of it happens to be identical to the implementation of the superclass TypeRepresentation::initInstance. You could either call the latter directly, or create a TypeRepresentation::initSingleInstance or something that you would call from both places (if you were worried that the superclass's implementation might not always be the right thing.)

Doesn't really matter that much, though. It isn't very much code.

Comment 9

[Niko Matsakis [:nmatsakis]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=804a94fb8dab

Comment 10

[Niko Matsakis [:nmatsakis]]

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

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d58ca9a622c0