898347 - [binary data] integrate binary data type descriptors into TI

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

In order to efficiently handle binary data objects, the JIT will need access to the type information for binary data objects. The plan is to put this information into the type objects, as described here:

http://smallcultfollowing.com/babysteps/blog/2013/07/19/integrating-binary-data-and-type-inference-in-spidermonkey/

This bug is specific to creating the type descriptors, storing them in the TI objects, and also using these same type objects to guide the interpreter code (which currently trawls through the JS objects themselves). A follow-up bug will target the jit integration.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Part 1: Add canonical type representations

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Part 2: Generalize newScript field of type objects

This patch generalizes the newScript field on type objects into an "addendum". In a later patch, I will add a second kind of addendum that can be associated with type objects for binary data types.

Comment 3

[Niko Matsakis [:nmatsakis]]

Attachment: Part 3: Add (but do not yet use) binary data addendum

Add a new kind of addendum to attach type repr's to a type object. Not yet used.

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Part 4: Convert binary data to use TypeRepresentations

Converts the binary data objects to keep an associated TypeRepresentation, which they use to determine sizes, alignments, offsets, etc.

Comment 5

[Niko Matsakis [:nmatsakis]]

Comment on attachment 784395 [details] [diff] [review]
Part 1: Add canonical type representations

Add TypeRepresentation objects that represent the canonical form of a binary data type. Eventually these objects will be exposed to self-hosted script, but for now they are used purely from C++ code.

Comment 6

[Niko Matsakis [:nmatsakis]]

Comment on attachment 784401 [details] [diff] [review]
Part 4: Convert binary data to use TypeRepresentations

Convert binary data code to use type representations. In some cases, cleanup semantics to be what the newer spec will require, for example reporting aligned sizes and offsets.

Comment 7

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

Comment on attachment 784395 [details] [diff] [review]
Part 1: Add canonical type representations

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

Looks great!

::: js/src/builtin/TypeRepresentation.cpp
@@ +149,5 @@
> +{
> +}
> +
> +static inline uint32_t alignTo(uint32_t value, uint32_t align) {
> +    return (value + align - 1) & -align;

Can you use AlignBytes() from jsutil.h?

@@ +226,5 @@
> +        (ScalarTypeRepresentation *) cx->malloc_(
> +            sizeof(ScalarTypeRepresentation));
> +    if (!ptr)
> +        return NULL;
> +    new(ptr) ScalarTypeRepresentation(kind, size, align);

Just out of curiosity, I'd like to know why you did this two step setup rather than js_new()?

::: js/src/builtin/TypeRepresentation.h
@@ +24,5 @@
> +  dummy JSObject. If you reference a TypeRepresentation*, you must
> +  ensure this dummy object is traced. In heap objects, this is
> +  typically done by invoking TypeRepresentation::trace, though another
> +  option is just to root the dummy object yourself.
> +

I think this should be rephrased as the dummy objects being managed by the GC, and the dummy's finalize deleting the TypedRepresentation. For a moment I was confused about how the TypeRepresentation instance itself was hooking into the GC.

@@ +51,5 @@
> +        Int32,
> +        Uint8,
> +        Uint16,
> +        Uint32,
> +        Float64,

are float32s captured by float64? Could you add a comment here.

@@ +81,5 @@
> +    uint32_t align() const { return size_; }
> +    Kind kind() const { return kind_; }
> +    JSObject *object() const { return pairedObject_.get(); }
> +
> +    static bool isTypeRepresenetation(HandleObject obj);

typo.

Comment 8

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

Comment on attachment 784401 [details] [diff] [review]
Part 4: Convert binary data to use TypeRepresentations

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

::: js/src/tests/ecma_6/BinaryData/architecture.js
@@ +31,5 @@
>      assertEq(Data.__proto__, Function.prototype);
>      assertEq(Data.prototype.__proto__, Object.prototype);
>      assertEq(Data.prototype.constructor, Data);
>      assertEq(typeof Data.prototype.update === "function", true);
>    

Please get rid of trailing space while you are here. Thanks!

@@ +34,5 @@
>      assertEq(typeof Data.prototype.update === "function", true);
>    
>      assertEq(Type.__proto__, Function.prototype);
>      assertEq(Type.prototype, Data);
>    

this too

@@ +43,5 @@
>          assertEq(numType.__proto__, Function.prototype);
>          assertEq(numType.bytes, sizes[i]);
>          assertThrows(function() new numType());
>      });
>    

and here.

::: js/src/tests/ecma_6/BinaryData/structtype.js
@@ +20,5 @@
>      assertEq(S.__proto__, StructType.prototype);
>      assertEq(S.prototype.__proto__, StructType.prototype.prototype);
>      assertEq(S.toString(), "StructType({x: int32, y: uint8, z: float64})");
>  
> +    assertEq(S.bytes, 16);

I thought binary data was supposed to stay platform independent as much as possible by not exposing implementation details like alignment. Is this required for asm.js/Emscripten?

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 784397 [details] [diff] [review]
Part 2: Generalize newScript field of type objects

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

::: js/src/jsinfer.cpp
@@ +4808,5 @@
>      const char *kind() { return "clearDefiniteGetterSetter"; }
>  
>      void newPropertyState(JSContext *cx, TypeSet *source)
>      {
> +        if (!object->newScript())

!object->hasNewScript()

@@ +5211,5 @@
>  
>      /* Strawman object to add properties to and watch for duplicates. */
>      state.baseobj = NewBuiltinClassInstance(cx, &JSObject::class_, gc::FINALIZE_OBJECT16);
>      if (!state.baseobj) {
> +        if (type->newScript())

type->hasNewScript()

@@ +5222,5 @@
>      if (!state.baseobj ||
>          state.baseobj->slotSpan() == 0 ||
>          !!(type->flags & OBJECT_FLAG_NEW_SCRIPT_CLEARED))
>      {
> +        if (type->newScript())

type->hasNewScript()

@@ +5232,5 @@
>       * If the type already has a new script, we are just regenerating the type
>       * constraints and don't need to make another TypeNewScript. Make sure that
>       * the properties added to baseobj match the type's definite properties.
>       */
> +    if (type->newScript()) {

type->hasNewScript()

@@ +6814,5 @@
>           * Properties and associated type sets for singletons are cleared on
>           * every GC. The type object is normally destroyed too, but we don't
>           * charge this to 'temporary' as this is not for GC heap values.
>           */
> +        JS_ASSERT(!hasNewScript());

JS_ASSERT(!addendum)

::: js/src/jsinfer.h
@@ +869,5 @@
>  };
>  
> +struct TypeNewScript;
> +
> +struct TypeObjectAddendum

This struct needs a brief comment.

@@ +875,5 @@
> +    enum Kind {
> +        NewScript
> +    };
> +
> +    Kind kind;

This member should be private.

@@ +1011,5 @@
> +
> +    /*
> +     * Returns addendum casted to a `TypeNewScript`. If there is no
> +     * addendum, returns NULL. If the addendum is of the wrong type,
> +     * asserts false.

This comment is too operational, it just parrots what the code does.  The semantics also don't really make sense, the method should be fallible or infallible and not some in between state.  Just make this:

TypeNewScript *newScript() {
    JS_ASSERT(hasNewScript());
    return addendum->asNewScript();
}

No comment is necessary.

Comment 10

[Brian Hackett [Laid off!]]

Comment on attachment 784399 [details] [diff] [review]
Part 3: Add (but do not yet use) binary data addendum

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

::: js/src/jsinfer.h
@@ +1051,5 @@
> +        TypeObjectAddendum *a = addendum;
> +        if (!a)
> +            return NULL;
> +        return a->asBinaryData();
> +    }

Same comments apply here as the previous patch.

::: js/src/jsinferinlines.h
@@ +1606,5 @@
>        case NewScript:
>          return TypeNewScript::writeBarrierPre(type->asNewScript());
> +
> +      case BinaryData:
> +        return TypeBinaryData::writeBarrierPre(type->asNewScript());

type->asBinaryData()?

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: Part 1: Add canonical type representations

Comment 12

[Niko Matsakis [:nmatsakis]]

Attachment: Part 2: Make TypedArrays use ScalarTypeRepresentation constants

Comment 13

[Niko Matsakis [:nmatsakis]]

Attachment: Part 3: Make BinaryData use TypeRepresentation

Also updates the names of the various type descriptors to be capitalized and adds support for clamped uint8.

Comment 14

[Niko Matsakis [:nmatsakis]]

Attachment: Part 4: Generalize NewScript field of type objects into addendum

Carrying over r+ from bhackett

Comment 15

[Niko Matsakis [:nmatsakis]]

Comment on attachment 784401 [details] [diff] [review]
Part 4: Convert binary data to use TypeRepresentations

Marking as obsolete due to new round of patches

Comment 16

[Niko Matsakis [:nmatsakis]]

Attachment: Part 5: Add BinaryDataAddendum

Comment 17

[Niko Matsakis [:nmatsakis]]

Attachment: Part 6: Add BinaryDataAddendum to binary data block objects

Comment 18

[Niko Matsakis [:nmatsakis]]

Attachment: Part 7: Optimize access to binary data struct fields of scalar type

This is a somewhat preliminary example of JIT integration, making use of the new TI information. In bug 898349 I intend to elaborate on the supported accesses.

Comment 19

[Niko Matsakis [:nmatsakis]]

Comment on attachment 789323 [details] [diff] [review]
Part 4: Generalize NewScript field of type objects into addendum

Marking as r? bhackett again; this is pretty similar to the patch you reviewed before, but I think enough has changed it might be worth looking over again.

Comment 20

[Niko Matsakis [:nmatsakis]]

OK, I just uploaded a 7 part series of patches that integrate binary data into TI and do some preliminary JIT integration to show how things are intended to work. More elaborate JIT integration to come in bug 898349. I also expect to do more elaborate unification, or at least *alignment* of binary data and typed arrays -- eventually binary data objects will have optional associated buffers, so they will have to interact with typed array views more "natively".

Comment 21

[Brian Hackett [Laid off!]]

Comment on attachment 789323 [details] [diff] [review]
Part 4: Generalize NewScript field of type objects into addendum

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

What is different about this patch?  If you're just making minor structural changes it isn't necessary to rereview.

Comment 22

[Brian Hackett [Laid off!]]

Comment on attachment 789326 [details] [diff] [review]
Part 6: Add BinaryDataAddendum to binary data block objects

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

::: js/src/builtin/BinaryData.cpp
@@ +2020,5 @@
>  {
>      JS_ASSERT(IsBinaryType(type));
>  
>      TypeRepresentation *typeRepr = typeRepresentation(cx, type);
> +    Class *classp = NULL;

The normal spelling for Class* instances is |clasp|

Comment 23

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

Comment on attachment 789318 [details] [diff] [review]
Part 1: Add canonical type representations

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

Waldo -- marking you as feedback? for the |typedef float float32_t| stuff, though really, you'd probably do a better job of reviewing this whole patch series. Unless you want to though, I'm not going to request the review be switched over to you, because I think I'll end up needing to understand this stuff anyway, and I don't want to subject Niko to double-review jeopardy. (Which is not to say that I'd object to you hijacking the review and me being secondary!)

::: js/src/builtin/TypeRepresentation.cpp
@@ +1,1 @@
> +#include "jscntxt.h"

MPL2 header

@@ +3,5 @@
> +#include "vm/Runtime.h"
> +#include "vm/StringBuffer.h"
> +#include "js/HashTable.h"
> +#include "TypeRepresentation.h"
> +#include "mozilla/HashFunctions.h"

separate subdir includes from toplevel includes, alphabetize.

@@ +12,5 @@
> +using namespace js;
> +using namespace mozilla;
> +
> +typedef float float32_t;
> +typedef double float64_t;

Whoa, having these sitting here bare like this makes me very nervous. Uh... can you at least static_assert that sizeof(float) == 4 and sizeof(double) == 8? I'm going to needinfo? Waldo on this to see if there is anything else that we should worry about.

@@ +15,5 @@
> +typedef float float32_t;
> +typedef double float64_t;
> +
> +///////////////////////////////////////////////////////////////////////////
> +// Class def'n for the dummy object

definition, and call it an "owner object"

@@ +47,5 @@
> +        return false;
> +
> +    switch (key1->kind()) {
> +      case TypeRepresentation::Scalar:
> +        return true;

Uh... doesn't type_ need to match? If so, you should have a test that checks this. If not, please comment why not. (Is it because type_ is from the interpretation of the type, and here you only care about the representation? Even so, wouldn't that mean that you should be checking the alignment and size as well?)

@@ +70,5 @@
> +    for (HashNumber i = 0; i < key1->fieldCount(); i++) {
> +        if (key1->field(i).id != key2->field(i).id)
> +            return false;
> +
> +        if (key1->field(i).typeRepr != key2->field(i).typeRepr)

This is always called after canonicalization? So I can only pass in typeReprs where I've already canonicalized the fields? I guess that's reasonable since you control how these pointers are produced, but can you do something like

  JS_ASSERT_IF(key1->field(i).typeRepr != key2->field(i).typeRepr, !match(key1->field(i).typeRepr, key2->field(i).typeRepr)

?

@@ +112,5 @@
> +HashNumber
> +TypeRepresentationHasher::hashStruct(StructTypeRepresentation *key)
> +{
> +    HashNumber hash = HashGeneric(key->kind());
> +    for (HashNumber i = 0; i < key->fieldCount(); i++) {

Why are you declaring 'i' to be of type HashNumber instead of uint32_t or whatever?

@@ +113,5 @@
> +TypeRepresentationHasher::hashStruct(StructTypeRepresentation *key)
> +{
> +    HashNumber hash = HashGeneric(key->kind());
> +    for (HashNumber i = 0; i < key->fieldCount(); i++) {
> +        hash = AddToHash(hash, JSID_BITS(key->field(i).id.get()));

Can you use JsidHasher::hash() from jsatom.h?

@@ +116,5 @@
> +    for (HashNumber i = 0; i < key->fieldCount(); i++) {
> +        hash = AddToHash(hash, JSID_BITS(key->field(i).id.get()));
> +        hash = AddToHash(hash, key->field(i).typeRepr);
> +    }
> +    return hash;

I don't know anything about binary data, but isn't there some way to specify alignment? If so, shouldn't that be taken into account? (Given that type_ isn't taken into account, but it influences alignment, it seems like there's something missing.)

@@ +157,5 @@
> +        size_ = align_ = 4;
> +        break;
> +
> +      case TYPE_FLOAT64:
> +        size_ = align_ = 8;

Man, I really need to read the binary data spec. It only works with binary data that follows these particular alignment rules?

@@ +171,5 @@
> +{
> +}
> +
> +static inline uint32_t alignTo(uint32_t value, uint32_t align) {
> +    return (value + align - 1) & -align;

s/value/address/ and JS_ASSERT(IsPowerOfTwo(align)).

@@ +227,5 @@
> +        (ScalarTypeRepresentation *) cx->malloc_(
> +            sizeof(ScalarTypeRepresentation));
> +    if (!ptr)
> +        return NULL;
> +    new(ptr) ScalarTypeRepresentation(type);

ScalarTypeRepresentation *ptr = cx->new_<ScalarTypeRepresentation*>(type);

@@ +256,5 @@
> +    if (!ptr)
> +        return NULL;
> +    new(ptr) ArrayTypeRepresentation(element, length);
> +    if (!rt->typeReprs.add(p, ptr)) {
> +        js_ReportOutOfMemory(cx);

Hm, if this were using RuntimeAllocPolicy, I don't think you'd need this.

@@ +299,5 @@
> +TypeRepresentation::trace(JSTracer *trace)
> +{
> +    // Note: do not visit the fields here, just push our dummy object
> +    // onto the mark stack. Only when our dummy object's trace
> +    // callback is called do we go and trace any type respresentations

*representations

@@ +302,5 @@
> +    // onto the mark stack. Only when our dummy object's trace
> +    // callback is called do we go and trace any type respresentations
> +    // that we reference. This prevents us from recursing arbitrarily
> +    // deep during the marking phase. It also ensures that we don't
> +    // retrace the same type representations many times in a row.

The comment seems a little too operational. I'd say something like:

A TypeRepresentation strongly holds its owner object, which strongly holds the TypeRepresentation's fields.

(Or "references" instead of "holds".) The rest is either implied or controlled externally (MarkObject might choose to immediately recurse.)

Come to think of it, maybe instead of asking you to call pairedObject_ ownerObject_, it should be fieldOwnerObject_.

Oh, wait a minute. I don't think I understand this ownership scheme after all. If I follow the tracing, it seems like my above comment is true -- but the dummy object's finalizer deletes the TypeRepresentation, not the fields. Your comment in the .h says that (TI) type objects point to the TypeRepresentation, whereas "binary data type descriptors" have the dummy object in a slot. What's a "binary data type descriptor"? That slot index is not in this patch. I guess a later patch would add it?

Right now, it seems to me that if typeobjects are the only thing involved, then you're ok because you'll trace everything from the typeobject, and when the typeobject goes away, then nothing else can access the repr. But if the mystery "binary data type descriptor" is the only thing holding a dummy object alive, then you have trouble at least with a moving GC: the dummy object will get traced, which will trace the repr's fields, so everything that should stay alive will. But the repr itself will never get traced, so if the dummy object is moved then the repr will point to a stale location. To patch this up, you'd need to make the dummy object tell the TypeRepresentation to mark the dummy object. Assuming I am understanding things correctly, though, this seems like a kind of dangerous setup -- the dummy object is sort of a strong reference to the repr, but not really because it only traces the fields and not the repr itself. It's sort of a "half-strong" reference.

I guess I'll need clarification before I can figure out which of my current patch comments are correct with respect to this dummy object stuff.

@@ +378,5 @@
> +    return false;
> +}
> +
> +/*static*/ const char *
> +ScalarTypeRepresentation::typeName(Type type)

Should this be #ifdef DEBUG, or is it used in production builds?

::: js/src/builtin/TypeRepresentation.h
@@ +1,1 @@
> +#ifndef builtin_TypeRepresentation_h

I think you need an MPL2 header here.

@@ +10,5 @@
> +  using `==`.
> +
> +  # Creation and canonicalization:
> +
> +  Each kind of `TypeRepresentation` object includes a `New` method

Other SM code seems to use Create instead of New. I think. Though on the other hand, Create() methods tend to always create, not return canonical pointers. Still, I think I'd prefer Create or GetCanonical.

@@ +18,5 @@
> +  be immutable, and the API permits only read access.
> +
> +  # Memory management:
> +
> +  Each TypeRepresentations is paired with a dummy JSObject. When this

s/TypeRepresentations/TypeRepresentation/

@@ +42,5 @@
> + */
> +
> +#include "js/HashTable.h"
> +#include "jspubtd.h"
> +#include "jsalloc.h"

Spidermonkey style says these should be sorted alphabetically, jsfoo.h first, and the subdirectory includes separated by a blank line:

#include "jsalloc.h"
#include "jspubtd.h"

#include "js/HashTable.h"

@@ +65,5 @@
> +    uint32_t size_;
> +    uint32_t align_;
> +    Kind kind_;
> +
> +    JSObject *makePairedObject(JSContext *cx);

This needs a comment, or better, put the purpose in the name. I think you could call this makeOwnerObject() and not need a comment, since you already described it nicely at the top.

@@ +79,5 @@
> +    void traceFields(JSTracer *tracer);
> +
> +  public:
> +    uint32_t size() const { return size_; }
> +    uint32_t align() const { return size_; }

return align_. There should be a test that would catch this.

Also, I'd rather you spelled out alignment() and alignment_ instead of abbreviating as 'align'. It's too much of a verb.

@@ +81,5 @@
> +  public:
> +    uint32_t size() const { return size_; }
> +    uint32_t align() const { return size_; }
> +    Kind kind() const { return kind_; }
> +    JSObject *object() const { return pairedObject_.get(); }

ownerObject() and ownerObject_

@@ +83,5 @@
> +    uint32_t align() const { return size_; }
> +    Kind kind() const { return kind_; }
> +    JSObject *object() const { return pairedObject_.get(); }
> +
> +    bool convertAndCopyTo(JSContext *cx, HandleValue from, uint8_t *mem);

Comment

@@ +85,5 @@
> +    JSObject *object() const { return pairedObject_.get(); }
> +
> +    bool convertAndCopyTo(JSContext *cx, HandleValue from, uint8_t *mem);
> +
> +    bool appendString(JSContext *cx, StringBuffer &buffer);

Comment. Until I read the implementation, I had no idea this was generating a descriptive string-form of the type.

@@ +87,5 @@
> +    bool convertAndCopyTo(JSContext *cx, HandleValue from, uint8_t *mem);
> +
> +    bool appendString(JSContext *cx, StringBuffer &buffer);
> +
> +    static bool isTypeRepresentation(HandleObject obj);

isTypeRepresentationOwnerObject, maybe? (The current name seems deceptive.)

@@ +90,5 @@
> +
> +    static bool isTypeRepresentation(HandleObject obj);
> +    static TypeRepresentation *of(HandleObject obj);
> +
> +    bool isScalar() {

const

@@ +94,5 @@
> +    bool isScalar() {
> +        return kind() == Scalar;
> +    }
> +
> +    ScalarTypeRepresentation *toScalar() {

Since this is just a cast, we seem to call these asScalar(). Also, can it be const? (Not return const, just be a const method.) jsobj.h has separate const-returning-const and non-const versions, so the non-const can't be const or it would be ambiguous. So unless I'm missing something, you should either make this const or provide a const-returning-const overload.

@@ +99,5 @@
> +        JS_ASSERT(isScalar());
> +        return (ScalarTypeRepresentation*) this;
> +    }
> +
> +    bool isArray() {

const

@@ +103,5 @@
> +    bool isArray() {
> +        return kind() == Array;
> +    }
> +
> +    ArrayTypeRepresentation *toArray() {

asArray

@@ +108,5 @@
> +        JS_ASSERT(isArray());
> +        return (ArrayTypeRepresentation*) this;
> +    }
> +
> +    bool isStruct() {

const

@@ +112,5 @@
> +    bool isStruct() {
> +        return kind() == Struct;
> +    }
> +
> +    StructTypeRepresentation *toStruct() {

asStruct

@@ +142,5 @@
> +    };
> +    static const int32_t TYPE_MAX = TYPE_UINT8_CLAMPED + 1;
> +
> +  private:
> +    friend class TypeRepresentation;

What is this for?

@@ +146,5 @@
> +    friend class TypeRepresentation;
> +
> +    Type type_;
> +
> +    ScalarTypeRepresentation(Type type);

Might be safer to mark this as explicit

@@ +148,5 @@
> +    Type type_;
> +
> +    ScalarTypeRepresentation(Type type);
> +
> +    bool appendStringScalar(JSContext *cx, StringBuffer &buffer);

Comment

@@ +185,5 @@
> +
> +    ArrayTypeRepresentation(TypeRepresentation *element,
> +                            uint32_t length);
> +
> +    void traceArrayFields(JSTracer *trace);

I'd probably go for traceFields().

@@ +205,5 @@
> +                         TypeRepresentation *elementTypeRepr,
> +                         uint32_t length);
> +};
> +
> +struct StructFieldPair {

"Pair" is just describing the representation. How about NamedStructField?

@@ +229,5 @@
> +    StructField fields_[];
> +
> +    StructTypeRepresentation(JSContext *cx,
> +                             AutoIdVector &ids,
> +                             AutoObjectVector &typeReprObjs);

typeReprOwners

@@ +231,5 @@
> +    StructTypeRepresentation(JSContext *cx,
> +                             AutoIdVector &ids,
> +                             AutoObjectVector &typeReprObjs);
> +
> +    void traceStructFields(JSTracer *trace);

traceFields, again?

@@ +251,5 @@
> +    const StructField *fieldNamed(HandleId id) const;
> +
> +    static JSObject *New(JSContext *cx,
> +                         AutoIdVector &ids,
> +                         AutoObjectVector &typeReprObjs);

typeReprOwners

@@ +273,5 @@
> +};
> +
> +typedef js::HashSet<TypeRepresentation *,
> +                    TypeRepresentationHasher,
> +                    SystemAllocPolicy> TypeRepresentationSet;

How much would it complicate things to use RuntimeAllocPolicy?

Comment 24

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

Comment on attachment 789319 [details] [diff] [review]
Part 2: Make TypedArrays use ScalarTypeRepresentation constants

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

Wow, these things show up everywhere now! I hadn't realized.

I sort of wonder whether these should live in a common place included by both the binary data stuff and the typed array stuff, instead of living with binary data, but maybe this is the direction things are moving in anyway. Either way, it's far better to have them in one place rather than two.

Comment 25

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

Comment on attachment 789321 [details] [diff] [review]
Part 3: Make BinaryData use TypeRepresentation

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

My confidence in this review is not high, but I couldn't detect anything wrong.

::: js/src/builtin/BinaryData.cpp
@@ +168,5 @@
> + * Given a user-visible type descriptor object, returns the
> + * dummy object for the TypeRepresentation* that we use internally.
> + */
> +static JSObject *
> +typeRepresentationObj(HandleObject typeObj)

typeRepresentationOwnerObject

@@ +178,5 @@
> +/*
> + * Given a user-visible type descriptor object, returns the
> + * TypeRepresentation* that we use internally.
> + *
> + * Note: this pointer is valid only so long as `typeObj` remains rooted.

Well, as long as it's alive, right? Maybe that's what you meant by "rooted", but given that typeObj is a HandleObject, the strong implication is that there's a RootedObject in scope. That's sufficient but not necessary -- this TypeRepresentation* will still be good if that particular root goes away, as long as the object is still alive for some other reason.

@@ +184,5 @@
> +static TypeRepresentation *
> +typeRepresentation(JSContext *cx, HandleObject typeObj)
> +{
> +    RootedObject typeReprObj(cx, typeRepresentationObj(typeObj));
> +    return TypeRepresentation::of(typeReprObj);

Hmm... does TypeRepresentation::of need a HandleObject? If not, it'd be better for it to take a bare JSObject*, eliminate the JSContext parameter here, and not have to worry about this being fallible. Otherwise, it seems like you're missing some null-checking.

@@ +212,3 @@
>  {
> +    TypeRepresentation *typeRepr = typeRepresentation(cx, type);
> +    return typeRepr->size();

I would lose the temporary. Its lifetime is a little scary anyway.

@@ +219,3 @@
>  {
> +    RootedObject type(cx, GetType(val));
> +    return TypeSize(cx, type);

Faster without the temporary, and one less place to fail.

@@ +452,2 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
> +    JSString *s = JS_NewStringCopyZ(cx, ScalarTypeRepresentation::typeName(N));

null check

@@ +1416,5 @@
> +    AutoObjectVector fieldTypeReprObjs(cx);
> +
> +    for (unsigned int i = 0; i < ids.length(); i++) {
> +        RootedValue fieldTypeVal(cx);
> +        RootedId id(cx, ids[i]);

Hoist these Rooted* declarations out of the loop, to save rooting overhead.

@@ +1508,5 @@
>      CallArgs args = CallArgsFromVp(argc, vp);
>  
>      if (argc >= 1 && args[0].isObject()) {
>          RootedObject structTypeGlobal(cx, &args.callee());
>          RootedObject fields(cx, args[0].toObjectOrNull());

You know args[0] is an object already, so use &args[0].toObject()

@@ +1635,5 @@
>          return JSObject::getGeneric(cx, proto, receiver, id, vp);
>      }
>  
> +    // Recover the original type object here (`field` contains only
> +    // its canonical form). The difference is of course user

s/of course//

@@ +1643,5 @@
> +    //     var Point2 = new StructType({x:uint8, y:uint8});
> +    //     var Line1 = new StructType({start:Point1, end: Point1});
> +    //     var Line2 = new StructType({start:Point2, end: Point2});
> +    //     var line1 = new Line1(...);
> +    //     var line2 = new Line1(...);

var line2 = new Line2(...);

@@ +2042,5 @@
> +                               cx->names().classPrototype, &protoVal))
> +        return NULL;
> +
> +    RootedObject obj(cx,
> +         NewObjectWithClassProto(cx, class_, protoVal.toObjectOrNull(), NULL));

The prototype has to be found, right? Make this &protoVal.toObject()

@@ +2098,5 @@
> +        return false;
> +
> +    if (argc == 1) {
> +        RootedValue initial(cx, args[0]);
> +        if (!ConvertAndCopyTo(cx, initial, obj))

I don't think you need the 'initial' temporary. Just use args[0] directly.

::: js/src/builtin/BinaryData.h
@@ +42,2 @@
>  enum BlockCommonSlots {
> +    // Refers to the type descriptor with which this block is associated.

s/Refers to//

@@ +44,5 @@
>      SLOT_DATATYPE = 0,
> +
> +    // If this value is NULL, then the block instance owns the
> +    // uint8_t* in its priate data. Otherwise, this field contains the
> +    // owner, and thus keeps the owner alive.

*private

@@ +105,5 @@
> +
> +    // creates zeroed memory of size of type
> +    static JSObject *createZeroed(JSContext *cx, HandleObject type);
> +
> +    // creates a block which aliases the memory owned by `owner`

s/which/that/ (sorry, I'm being super picky)

@@ +115,5 @@
> +    static JSBool construct(JSContext *cx, unsigned int argc, jsval *vp);
> +
> +};
> +
> +/* This represents the 'a' and it's [[Prototype]] chain */

*its

the 'a'?

Comment 26

[Niko Matsakis [:nmatsakis]]

Some initial responses:

> Whoa, having these sitting here bare like this makes me very nervous. Uh... can you at least
> static_assert that sizeof(float) == 4 and sizeof(double) == 8? I'm going to needinfo? Waldo
> on this to see if there is anything else that we should worry about.

Sorry, this is a holdover from an older version of the code and has been removed. I'll upload an updated patch. But the static_asserts are a good idea.

> I'd probably go for traceFields().

I did not call it this because it would shadow the name from the base class. I personally am fine with doing that but I thought people might complain. But I'm happy to change it to shadow.

> > +  private:
> > +    friend class TypeRepresentation;
>
> What is this for?

The base class is friends with the subclasses so that it can invoke the specialized versions of the trace routines etc, which are declared as private to keep them local to the file. I could also just make those methods virtual, which I was avoiding for some reason.

> This is always called after canonicalization? So I can only pass in typeReprs where
> I've already canonicalized the fields? 

Yes. It is in fact impossible to get ahold of a TypeRepresentation* that has not been canonicalized (those are local to the creation and hashing routines).

Comment 27

[Niko Matsakis [:nmatsakis]]

> Man, I really need to read the binary data spec. It only works with binary data that
> follows these particular alignment rules?

Yes, at least as of now, the spec mandates particular alignments.

Comment 28

[Niko Matsakis [:nmatsakis]]

> I guess I'll need clarification before I can figure out which of my current patch
> comments are correct with respect to this dummy object stuff.

I've since realized that the setup is not as complicated as I made it sound. Essentially, you can think of it like this:

- Each canonical type definition results in an (internal) type representation object.
- Type representation objects have, as their private data, a TypeRepresentation*.

Any place that references a type representation traces it in the normal way: by marking the type representation object. The only "tricky" part is that -- for convenience, stronger typing and to avoid the need to make the keys to hashtables cope with a moving GC -- we sometimes store the TypeRepresentation* instead of a HeapObject. Nonetheless, when tracing, we just mark the owner object of this TypeRepresentation* rather than eagerly tracing it.

In more detail, tracing works like:

- Type representation objects have a custom trace method that walks this TypeRepresentation*
  and marks all the type representation objects that it finds.
- Type representation objects are referenced from two places:
  - TI Type Objects, which store a TypeRepresentation* and which mark its owner object
  - Binary data type descriptors, which store the type representation object in one of
    their reserved fields and hence do not require custom tracing logic

Currently, these type representation objects are never referenced from script of any kind and thus are only used to give the GC something to trace and collect (hence the term "dummy object"). However, I plan to migrate a lot of the binary data helper functions to self-hosted code eventually, in which case the type representation objects will be used there to gain access to specialized intrinsic operations like memcpy etc.

Does this make more sense? If so, I will update the comment in the file accordingly.

Comment 29

[Niko Matsakis [:nmatsakis]]

I'm marking this as needinfo? sfink to get clarification on whether that description of the memory management scheme is more understandable (and of course if there are any flaws in it)

Comment 30

[Niko Matsakis [:nmatsakis]]

> ScalarTypeRepresentation *ptr = cx->new_<ScalarTypeRepresentation*>(type);

I remember now why I did not do this: the constructors are private, and hence this form does not work. I want the constructors to be private because it ensures that there are no non-canonical instances floating about.

Comment 31

[Niko Matsakis [:nmatsakis]]

> @@ +219,3 @@
> >  {
> > +    RootedObject type(cx, GetType(val));
> > +    return TypeSize(cx, type);
>
> Faster without the temporary, and one less place to fail.

I don't understand this comment (and a few others like it). Is rooting fallible? Based on reading the source for Rooted<T>, it doesn't appear to be -- it uses the stack for allocation.

Comment 32

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

(In reply to Niko Matsakis [:nmatsakis] from comment #28)
> > I guess I'll need clarification before I can figure out which of my current patch
> > comments are correct with respect to this dummy object stuff.
> 
> I've since realized that the setup is not as complicated as I made it sound.
> Essentially, you can think of it like this:
> 
> - Each canonical type definition results in an (internal) type
> representation object.
> - Type representation objects have, as their private data, a
> TypeRepresentation*.
> 
> Any place that references a type representation traces it in the normal way:
> by marking the type representation object. The only "tricky" part is that --
> for convenience, stronger typing and to avoid the need to make the keys to
> hashtables cope with a moving GC -- we sometimes store the
> TypeRepresentation* instead of a HeapObject. Nonetheless, when tracing, we
> just mark the owner object of this TypeRepresentation* rather than eagerly
> tracing it.
> 
> In more detail, tracing works like:
> 
> - Type representation objects have a custom trace method that walks this
> TypeRepresentation*
>   and marks all the type representation objects that it finds.
> - Type representation objects are referenced from two places:
>   - TI Type Objects, which store a TypeRepresentation* and which mark its
> owner object
>   - Binary data type descriptors, which store the type representation object
> in one of
>     their reserved fields and hence do not require custom tracing logic
> 
> Currently, these type representation objects are never referenced from
> script of any kind and thus are only used to give the GC something to trace
> and collect (hence the term "dummy object"). However, I plan to migrate a
> lot of the binary data helper functions to self-hosted code eventually, in
> which case the type representation objects will be used there to gain access
> to specialized intrinsic operations like memcpy etc.
> 
> Does this make more sense? If so, I will update the comment in the file
> accordingly.

Well, I guess that's what I understood. I think my confusion stems from the fact that a type representation object traces a TypeRepresentation's fields, but its finalizer frees the TypeRepresentation itself. In my primitive brain, I expect that if an object "owns" some data, then it keeps it alive (by marking/tracing it) and discards it (in its finalizer). Your owner objects are tracing and discarding different things.

Whether that's a problem in practice is a separate question. I don't think it's a leak, because if you ignore type TypeRepresentations and only consider the type repr objects (with pointers to the field objects, which happen to be stored in an auxiliary area, namely the TypeRepresentation), then the graph is traced properly and the right objects are kept alive (and the right objects are discarded.) The finalizers handle the auxiliary storage properly (ie, the TypeRepresentations.) So that's all fine.

But the pointer from the TypeRepresentation to the repr object seems more problematic. With moving GC, every edge must be visited, and that doesn't seem to be guaranteed. Specifically, if you ignore TI type objects, and only start your traversal from a repr object, it doesn't seem like the edge from the TypeRepresentation to its repr object is ever marked. (The object itself is marked correctly, because it is found through a different edge. But we have to visit all edges with a moving GC, not just all gcthings.)

Is that analysis correct? If so, then if I conceptualize it as: the repr objects are the primary things to consider, and the TypeRepresentations are their private data that store indirect pointers to other repr objects (they point directly to another TypeRepresentation, which has an ownerObject pointer). Then the natural fix is for a TI type object to trace its stuff by retrieving the repr object pointer and marking that (well, pushing it on the mark stack). It doesn't have to do it directly; it can call a method on the TypeRepresentation or whatever. So all marking and tracing is conceptually controlled by the gcthings.

Alternatively, you could make the TypeRepresentations store direct pointers to their fields' repr objects. But that would slow down regular lookup/traversal, so it doesn't seem right.

The spot fix, which probably boils down to exactly the same thing, is for the repr object's obj_trace to mark itself through the TypeRepresentation's edge. (I say "mark", but I really mean "update the pointer if necessary and do nothing else." Our terminology is all tangled with respect to marking, tracing, visiting, and updating.)

Comment 33

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

(In reply to Niko Matsakis [:nmatsakis] from comment #30)
> > ScalarTypeRepresentation *ptr = cx->new_<ScalarTypeRepresentation*>(type);
> 
> I remember now why I did not do this: the constructors are private, and
> hence this form does not work. I want the constructors to be private because
> it ensures that there are no non-canonical instances floating about.

Ok, fair enough. Comment, then?

Comment 34

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

(In reply to Niko Matsakis [:nmatsakis] from comment #31)
> > @@ +219,3 @@
> > >  {
> > > +    RootedObject type(cx, GetType(val));
> > > +    return TypeSize(cx, type);
> >
> > Faster without the temporary, and one less place to fail.
> 
> I don't understand this comment (and a few others like it). Is rooting
> fallible? Based on reading the source for Rooted<T>, it doesn't appear to be
> -- it uses the stack for allocation.

Oh, you're right. I just tend to assume that anything with a cx parameter is fallible. Which is partly why I want to be sure that cx is only passed through when necessary, since almost everything that takes a leading cx parameter can GC, and anything that demands a Handle can also GC (otherwise, what's the point of the indirection?)

Comment 35

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

Comment on attachment 789327 [details] [diff] [review]
Part 7: Optimize access to binary data struct fields of scalar type

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

I'm no JIT expert, but this all seems like pretty straightforward cut & paste, so I'll go out on a limb and say it's ok.

::: js/src/ion/IonBuilder.cpp
@@ +7768,5 @@
> +    // All objects in type set must correspond to a binary data type
> +    // repr that has the same field, at the same offset, with the same
> +    // result type.
> +    //
> +    // FIXME--An improvement would be not to require the *same*

File a bug, put the bug number here.

@@ +7774,5 @@
> +    // generalization of `fieldTypeRepr`, such as a common prefix.
> +    TypeRepresentation *fieldTypeRepr = NULL;
> +    size_t fieldOffset;
> +    for (uint32_t i = 0; i < objTypes->getObjectCount(); i++) {
> +        RootedTypeObject type(cx, objTypes->getTypeObject(0));

Pull the RootedTypeObject decl out of the loop. Uh... and shouldn't this be getTypeObject(i)? If so, a test should have caught this.

@@ +7803,5 @@
> +        } else if (structField->typeRepr != fieldTypeRepr ||
> +                   structField->offset != fieldOffset) {
> +            // Two type reprs that have the same field, but at a
> +            // different offset or with incompatible types.
> +            return NULL;

return true. And a test for this case, if you don't have one already.

::: js/src/jit-test/tests/binary-data/jit-read-int.js
@@ +2,5 @@
> +                                y: Float64,
> +                                z: Float64});
> +
> +function foo() {
> +  print("hi");

Hi!

Comment 36

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

Comment on attachment 789318 [details] [diff] [review]
Part 1: Add canonical type representations

Sounds like the thing I wanted feedback on is gone, so clearing the flag.

Comment 37

[Niko Matsakis [:nmatsakis]]

> Well, I guess that's what I understood. I think my confusion stems from the fact that a type
> representation object traces a TypeRepresentation's fields, but its finalizer frees the
> TypeRepresentation itself. In my primitive brain, I expect that if an object "owns" some data,
> then it keeps it alive (by marking/tracing it) and discards it (in its finalizer). Your owner
> objects are tracing and discarding different things.

I see your point. I think there is a bug, but it's very simply fixed: the traceFields() routine should simply mark the "self-reference", which is more-or-less what you suggest at the end. In other words, my traceFields() routine simply wasn't tracing out the complete contents of the object, and that is clearly wrong.

In particular, I think your "primitive brain explanation" is correct. The TypeRepresentation's object owns the TypeRepresentation*, and it will trace it and free it as appropriate. The lifetime of that *owner object* is controlled by the GC, and thus by type objects and others who reference it.

Comment 38

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

(In reply to Niko Matsakis [:nmatsakis] from comment #26)
> Some initial responses:
> 
> > Whoa, having these sitting here bare like this makes me very nervous. Uh... can you at least
> > static_assert that sizeof(float) == 4 and sizeof(double) == 8? I'm going to needinfo? Waldo
> > on this to see if there is anything else that we should worry about.
> 
> Sorry, this is a holdover from an older version of the code and has been
> removed. I'll upload an updated patch. But the static_asserts are a good
> idea.
> 
> > I'd probably go for traceFields().
> 
> I did not call it this because it would shadow the name from the base class.
> I personally am fine with doing that but I thought people might complain.
> But I'm happy to change it to shadow.

Oh, I see. No, your original names are better. Shadowing is bad.

> > > +  private:
> > > +    friend class TypeRepresentation;
> >
> > What is this for?
> 
> The base class is friends with the subclasses so that it can invoke the
> specialized versions of the trace routines etc, which are declared as
> private to keep them local to the file. I could also just make those methods
> virtual, which I was avoiding for some reason.

Well, it expands the size with a vtable pointer, which is a good reason to not do it. The friend thing is fine, I was just wondering.

Comment 39

[Brian Hackett [Laid off!]]

Comment on attachment 789323 [details] [diff] [review]
Part 4: Generalize NewScript field of type objects into addendum

Canceling review per comment 21.

Comment 40

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898347-Part0.diff

Comment 41

[Niko Matsakis [:nmatsakis]]

Attachment: Part 1: Add canonical type representations

Comment 42

[Niko Matsakis [:nmatsakis]]

Attachment: Part 2: Make TypedArrays use ScalarTypeRepresentation constants

Carrying over r+ from sfink

Comment 43

[Niko Matsakis [:nmatsakis]]

Attachment: Part 3: Make BinaryData use TypeRepresentation

Carrying over r+ from sfink.

Asking for quick feedback from Waldo specifically concerning the binary block operations like getproperty, getelement etc, which are to be found at the end of the patch. Can you just cast an eye over those and check if you see any obvious problems? I tried to follow the model of typed arrays, but in some cases (e.g., obj_delete*), the typed array code seemed just to delegate which is not appropriate here.

Comment 44

[Niko Matsakis [:nmatsakis]]

Attachment: Part 4: Generalize NewScript field of type objects into addendum

Carrying over r+ from bhackett

Comment 45

[Niko Matsakis [:nmatsakis]]

Attachment: Part 5: Add BinaryDataAddendum

Carrying over r+ from bhackett

Comment 46

[Niko Matsakis [:nmatsakis]]

Attachment: Part 6: Add BinaryDataAddendum to binary data block objects

Carrying over r+ from bhackett

Comment 47

[Niko Matsakis [:nmatsakis]]

Comment on attachment 789327 [details] [diff] [review]
Part 7: Optimize access to binary data struct fields of scalar type

Marking as obsolete; more extensive testing revealed some crashes that seemed to be specific to this patch. I'll fix those up and move this patch over to bug 898349.

Comment 48

[Niko Matsakis [:nmatsakis]]

Comment on attachment 792328 [details] [diff] [review]
Part 1: Add canonical type representations

This is still missing a static assert to check that the C types have the same size and alignment as the specification. I have to figure out the best way to achieve that. Otherwise, I tried to address the various nits and renames, as well as the issue of type descriptors tracing their own self-pointer.

Comment 49

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

Comment on attachment 792328 [details] [diff] [review]
Part 1: Add canonical type representations

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

::: js/src/builtin/TypeRepresentation.cpp
@@ +19,5 @@
> +using namespace js;
> +using namespace mozilla;
> +
> +///////////////////////////////////////////////////////////////////////////
> +// Class def'n for the owner object

s/def'n/definition/

@@ +51,5 @@
> +        return false;
> +
> +    switch (key1->kind()) {
> +      case TypeRepresentation::Scalar:
> +        return true;

So this is structural only, and doesn't need to consider the type or size? If so, a comment explaining why would be good.

::: js/src/builtin/TypeRepresentation.h
@@ +16,5 @@
> +  using `==`.
> +
> +  # Creation and canonicalization:
> +
> +  Each kind of `TypeRepresentation` object includes a `New` method

`Create`

@@ +24,5 @@
> +  designed to be immutable, and the API permits only read access.
> +
> +  # Memory management:
> +
> +  Each TypeRepresentations has a representative JSObject, called its

s/TypeRepresentations/TypeRepresentation/

@@ +90,5 @@
> +    JSObject *ownerObject() const { return ownerObject_.get(); }
> +
> +    // Appends a stringified form of this type representation onto
> +    // buffer, for use in error messages and the like.
> +    bool appendString(JSContext *cx, StringBuffer &buffer);

sorry for the churn, but could you call this appendDescription? Or just describe() would be ok too. (I keep stumbling over this because typerepr.appendString(...) suggests to me that it's modifying the TypeRepresentation.)

@@ +92,5 @@
> +    // Appends a stringified form of this type representation onto
> +    // buffer, for use in error messages and the like.
> +    bool appendString(JSContext *cx, StringBuffer &buffer);
> +
> +    static bool isTypeRepresentationOwnerObject(JSObject *obj);

I think you may later want to split out an actual TypeRepresentationOwnerObject class, and then this would be is<TypeRepresentationOwnerObject>. It would already work if you said is<TypeRepresentation>, but that's confusing since TypeRepresentation is not a JSObject, so I'd rather you not do this.

So nothing to do now, but I suspect in the future when you expose the owner object more, you may end up wanting the naming to be TypeRepresentationObject (a JSObject subclass) with a private field holding the TypeRepresentation (same as now.) The owner object is gradually going from being a dummy thing on the side just for GC purposes, to the primary interface.

@@ +155,5 @@
> +
> +    explicit ScalarTypeRepresentation(Type type);
> +
> +    // See TypeRepresentation::appendString()
> +    bool appendStringScalar(JSContext *cx, StringBuffer &buffer);

describeScalar or appendScalarDescription?

@@ +195,5 @@
> +    // See TypeRepresentation::traceFields()
> +    void traceArrayFields(JSTracer *trace);
> +
> +    // See TypeRepresentation::appendString()
> +    bool appendStringArray(JSContext *cx, StringBuffer &buffer);

describeArray or appendArrayDescription?

@@ +237,5 @@
> +    // See TypeRepresentation::traceFields()
> +    void traceStructFields(JSTracer *trace);
> +
> +    // See TypeRepresentation::appendString()
> +    bool appendStringStruct(JSContext *cx, StringBuffer &buffer);

describeStruct or appendStructDescription?

@@ +260,5 @@
> +struct TypeRepresentationHasher
> +{
> +    typedef TypeRepresentation *Lookup;
> +    static HashNumber hash(TypeRepresentation *key);
> +    static bool match(TypeRepresentation *key1, TypeRepresentation *key2);

If this is a structural match that ignores scalar size and type_, it should be commented here.

Comment 50

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898347-Part0.diff

Comment 51

[Niko Matsakis [:nmatsakis]]

Attachment: Part 1: Add canonical type representations

Carrying over r+ from sfink

Comment 52

[Niko Matsakis [:nmatsakis]]

Attachment: Part 2: Make TypedArrays use ScalarTypeRepresentation constants

Comment 53

[Niko Matsakis [:nmatsakis]]

Attachment: Part 3: Make BinaryData use TypeRepresentation

Comment 54

[Niko Matsakis [:nmatsakis]]

Attachment: Part 4: Generalize NewScript field of type objects into addendum

Carrying over r+ from bhackett

Comment 55

[Niko Matsakis [:nmatsakis]]

Attachment: Part 5: Allow TypeRepresentations objects to be attached to TI objects

Carrying over r+ from bhackett

Comment 56

[Niko Matsakis [:nmatsakis]]

Attachment: Part 6: Add BinaryDataAddendum to binary data block objects

Comment 57

[Niko Matsakis [:nmatsakis]]

Comment on attachment 794798 [details] [diff] [review]
Part 6: Add BinaryDataAddendum to binary data block objects

Adding f? decoder / gkw to fuzz this series of patches

Comment 58

[Niko Matsakis [:nmatsakis]]

Attachment: All patches rolled together

This is the same material as the previous patches, but rolled together and rebased against mozilla central (changeset fa56d4c9e630).

Comment 59

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 794837 [details] [diff] [review]
All patches rolled together

function eval() {
    yield(undefined)
}
new(StructType)
(eval())

Crash [@ generator_finalize]

===

new StructType([])

Assertion failure: clasp != &ArrayObject::class_, at jsobj.cpp

Comment 60

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Testcases in comment 59 were tested on Mac 10.8 on a js debug 64-bit threadsafe deterministic shell.

Comment 61

[Niko Matsakis [:nmatsakis]]

Attachment: All patches rolled together

Address problem cases identified by decoder. Based against m-c rev 01576441bdc6.

Comment 62

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Niko Matsakis [:nmatsakis] from comment #61)
> Created attachment 795111 [details] [diff] [review]
> All patches rolled together
> 
> Address problem cases identified by decoder. Based against m-c rev
> 01576441bdc6.

I assume my testcases were fixed as well?

Comment 63

[Christian Holler (:decoder)]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #62)
> 
> I assume my testcases were fixed as well?


I think he was referring to you, I didn't provide any testcasey yet. I'll probably start testing today.

Comment 64

[Niko Matsakis [:nmatsakis]]

Sorry, yes, addressing testcases by gkw.

Comment 65

[Christian Holler (:decoder)]

Comment on attachment 795111 [details] [diff] [review]
All patches rolled together

Found three crashes so far, no special options required:


this.__proto__ =  Proxy.create({});
new StructType;


Crashes at [@ js::TypeRepresentation::makeOwnerObject].

===

var A = new ArrayType(uint8, 10);
var a = new A();
a.forEach(function(val, i) {
  assertEq(arguments[5], a);
});


Assertion failure: index == typeRepr->asArray()->length(), at js/src/builtin/BinaryData.cpp:2337



===

var Color = new StructType({r: uint8, g: uint8, b: uint8});
var white2 = new Color({r: 255, toString: null, b: 253});


Crashes at [@ JS_EncodeString].

Comment 66

[Christian Holler (:decoder)]

A few more crashes:

new ArrayType(uint8, .0000000009);


=> Assertion failure: isInt32(), at ./dist/include/js/Value.h:1093

===

var Vec3 = new ArrayType(float32, 3);
var Sprite = new ArrayType(Vec3, 3);
var mario = new Sprite();
mario[/\u00ee[]/] = new Vec3([1, 0, 0]);


=> Assertion failure: IsStructType(type), at js/src/builtin/BinaryData.cpp:1634

===

var AA = new ArrayType(new ArrayType(uint8, (2147483647)), 5);
var aa = new AA();
var aa0 = aa[0];


=> Assertion failure: offset + TypeSize(cx, type) <= BlockSize(cx, owner), at js/src/builtin/BinaryData.cpp:1728

Comment 67

[Niko Matsakis [:nmatsakis]]

Attachment: All patches rolled together

Rebased off of m-c rev 14b1e8c2957e.

Comment 68

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 795624 [details] [diff] [review]
All patches rolled together

Nothing more found overnight.

Comment 69

[Christian Holler (:decoder)]

Comment on attachment 795624 [details] [diff] [review]
All patches rolled together

Looks good :)

Comment 70

[Niko Matsakis [:nmatsakis]]

Attachment: Bug898347-Part0.diff

Comment 71

[Niko Matsakis [:nmatsakis]]

Attachment: Part 1: Add canonical type representations

Carrying over r+ from sfink

Comment 72

[Niko Matsakis [:nmatsakis]]

Comment on attachment 794792 [details] [diff] [review]
Part 1: Add canonical type representations

Forgot to mark this obsolete.

Comment 73

[Niko Matsakis [:nmatsakis]]

Attachment: Part 2: Make TypedArrays use ScalarTypeRepresentation constants

Carrying over r+ from sfink

Comment 74

[Niko Matsakis [:nmatsakis]]

Attachment: Part 3: Make BinaryData use TypeRepresentation

Carrying over r+ from sfink

Comment 75

[Niko Matsakis [:nmatsakis]]

Attachment: Part 4: Generalize NewScript field of type objects into addendum

Carrying over r+ from sfink

Comment 76

[Niko Matsakis [:nmatsakis]]

(Correction: from bhackett)

Comment 77

[Niko Matsakis [:nmatsakis]]

Attachment: Part 5: Add BinaryDataAddendum

Carrying over r+ from bhackett

Comment 78

[Niko Matsakis [:nmatsakis]]

Attachment: Part 6: Add BinaryDataAddendum to binary data block objects

Carrying over r+ from bhackett

Comment 79

[Niko Matsakis [:nmatsakis]]

Uploaded rebased patches. Successful fuzzing (see comments above). Successful try run: https://tbpl.mozilla.org/?tree=Try&rev=fb96874c4e32

Comment 80

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b68259eb4307
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/acc3e8dacecc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd15bcd90326
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/877660fcfde5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5602962dbb21
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/847a9330f30c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/10f121af2f56

Comment 81

[Niko Matsakis [:nmatsakis]]

Attachment: Patch up the test_interfaces test to reflect re-enabling of binary data

Comment 82

[Niko Matsakis [:nmatsakis]]

Comment on attachment 796769 [details] [diff] [review]
Patch up the test_interfaces test to reflect re-enabling of binary data

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

r+ from evilpie over IRC

Comment 83

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5946208b0507

Comment 84

[Niko Matsakis [:nmatsakis]]

Attachment: Correct mistake in previous patch

Comment 85

[Niko Matsakis [:nmatsakis]]

Comment on attachment 796829 [details] [diff] [review]
Correct mistake in previous patch

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

r+ from evilpie on IRC

Comment 86

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a2cd23f6187a

Comment 87

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b68259eb4307
https://hg.mozilla.org/mozilla-central/rev/acc3e8dacecc
https://hg.mozilla.org/mozilla-central/rev/cd15bcd90326
https://hg.mozilla.org/mozilla-central/rev/877660fcfde5
https://hg.mozilla.org/mozilla-central/rev/5602962dbb21
https://hg.mozilla.org/mozilla-central/rev/847a9330f30c
https://hg.mozilla.org/mozilla-central/rev/10f121af2f56
https://hg.mozilla.org/mozilla-central/rev/5946208b0507
https://hg.mozilla.org/mozilla-central/rev/a2cd23f6187a