966575 - Remove most uses of TypeRepresentation and replace with TypeDescr

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Typed objects currently keep a pointer to a TypeRepresentation that represents a canonical form of their layout. When I originally coded that up, I tried to write all of the typed object logic in terms of type representations -- but it turns out that it is not enough to track the canonical form of the layout, you wind up having to track precisely which user-defined type descriptor is in use as well. That is, if there are two distinct StructType instances with equivalent layouts, we need to know which of the two we are dealing with in order to create types with the proper prototypes. Therefore, the TypeRepresentation winds up being more of a distraction (and extra, unnecessary object) than anything else.

I have a series of patches that removes *almost* all uses of TypeRepresentations. In the process, it revamps the TypedObject code to use a more modern SpiderMonkey style based on is<T>() and as<T>() and adding methods to subtypes of JSObject (a style I didn't fully understand before).

The one remaining use of type representations after these patches is to serve as a hash key so that we can cheaply test whether two types are equivalent. I would like to remove this too by simply having the type descriptors themselves be the keys in the equivalence hashtable, but I haven't figured out how to have a hashtable with JSObject keys yet.

Still, given that this touches so many lines of code, I'd like to get these patches landing soon so I can build off of them without painful rebasing.

In the process, I also repeated an older renaming from TypeObject to TypeDescr, because the former is so easily confused with TI type objects. I know it would be better to rename TI type objects, but I have opted to use descr as the name here because it is more unique and suggestive. The spec simply refers to these objects as types which is quite generic -- I would prefer to modify the spec as well to adopt the "descriptor" terminology.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part01.diff

- Change TypedObject code to use is<> and as<>, rather than the static helpers I had been using before. 

- Introduce the Descr name for type objects to help reduce ambiguity and overloading on the terms "type" and "object" (possibly the two most overloaded terms in CS).

- Convert from generic JSObject* parameters to more specific types, avoiding the need for numerous assertions and helping to make the code more type safe.

Comment 2

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

Comment on attachment 8369072 [details] [diff] [review]
Bug966575-Part01.diff

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

::: js/src/builtin/TypedObject.cpp
@@ +984,1 @@
>                         Handle<GlobalObject *> global,

I think the indentation may be off by one here.

@@ +1024,1 @@
>                       Handle<GlobalObject*> global,

indentation

::: js/src/builtin/TypedObject.h
@@ +222,5 @@
>  };
>  
>  /*
> + * Properties and methods of the `ArrayType` meta type object. There
> + * is no `class_` field because we simply `ArrayType` is a native

"...because we simply ArrayType is..."?

@@ +294,5 @@
>  };
>  
>  /*
> + * Properties and methods of the `StructType` meta type object. There
> + * is no `class_` field because we simply `StructType` is a native

Same mangled sentence comment

Comment 3

[Niko Matsakis [:nmatsakis]]

Marking as [leave-open] since I did this work in many steps and I plan to land them one by one.

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part02.diff

Intermediate step that updates the code to not track a TypeRepresentation / descriptor separately (for the most part) but instead just track a descriptor. The data which we used to fetch from the TypeRepresentation we now fetch in two steps: loading the TypeRepresentation and then fetching the slot from that. Later patches will move those slots into the descriptor itself.

Comment 5

[Niko Matsakis [:nmatsakis]]

Try run for part 1: https://tbpl.mozilla.org/?tree=Try&rev=1f6f1f08d032

Comment 6

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e6a7d3372dee (Part 1)

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part03.diff

Comment 9

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part04.diff

Comment 10

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part05.diff

Comment 11

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part06.diff

Comment 12

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part07.diff

Comment 13

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part08.diff

Comment 14

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part09.diff

Comment 15

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part10.diff

Comment 16

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part11.diff

Comment 17

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part12.diff

Comment 18

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

Comment on attachment 8370896 [details] [diff] [review]
Bug966575-Part03.diff

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

I'm attempting to follow along with the spec and what's happening here, but I can't say I really understand all this.

::: js/src/builtin/TypedObject.js
@@ +231,5 @@
> +TypedObjectPointer.prototype.moveToArray = function(propName, length) {
> +  // For an array, property must be an element. Note that we use the
> +  // length as loaded from the type *representation* as opposed to
> +  // the type *object*; this is because some type objects represent
> +  // unsized arrays and hence do not have a length.

This comment refers to type representations, not descriptors, and isn't really valid here anymore (since the length is passed in, and comes from different sources.) Maybe move it up to the caller?

@@ +272,5 @@
>           "moveToField invoked on non-struct");
>    assert(HAS_PROPERTY(this.descr.fieldTypes, propName),
>           "moveToField invoked with undefined field");
>  
> +  // FIXME -- opaque??

Bug #?

Comment 19

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

Comment on attachment 8370900 [details] [diff] [review]
Bug966575-Part04.diff

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

::: js/src/builtin/TypedObject.cpp
@@ +1307,3 @@
>              {
>                  return nullptr;
>              }

now the extra brackets are unneeded

::: js/src/jsinfer.cpp
@@ +4640,5 @@
>      JS_ASSERT(!unknownProperties());
>  
>      if (addendum) {
>          JS_ASSERT(hasTypedObject());
> +        JS_ASSERT(&typedObject()->descr() == &*descr);

My compiler didn't need any of these &* things (except for the one at the end of TypedDatum::createUnattachedWithClass). They make me generally nervous, since they mainly just tell the compiler to forget about a pointer being rooted, though I suppose the rooting analysis would complain if that actually did anything.

@@ +4668,2 @@
>    : TypeObjectAddendum(TypedObject),
> +    descr_(&*descr)

same

@@ +4672,5 @@
> +
> +TypeDescr &
> +js::types::TypeTypedObject::descr() {
> +    return descr_->as<TypeDescr>();
> +}

(Note that with just the patches up to this point applied, I needed to #include builtin/TypedObject.h in jsinfer.cpp to get it to compile.)

Comment 20

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

Comment on attachment 8370902 [details] [diff] [review]
Bug966575-Part06.diff

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

::: js/src/builtin/TypedObject.cpp
@@ +938,5 @@
> +    size_t l = fieldNames.getDenseInitializedLength();
> +    for (size_t i = 0; i < l; i++) {
> +        JSAtom &a = fieldNames.getDenseElement(i).toString()->asAtom();
> +        if (JSID_IS_ATOM(id, &a)) {
> +            *out = i;

Ugh, a linear search. I hope we don't do this too often.

::: js/src/builtin/TypedObject.h
@@ +341,5 @@
> +    // Set `*out` to the index of the field named `id` and returns true,
> +    // or returns false if no such field exists.
> +    bool fieldIndex(jsid id, size_t *out);
> +
> +    // Returns the type descr of the field at index `index`

Previous comment was imperative. I prefer imperative. Change this to "Return" and end with a period.

@@ +344,5 @@
> +
> +    // Returns the type descr of the field at index `index`
> +    SizedTypeDescr &fieldDescr(size_t index);
> +
> +    // Returns the offset of the field at index `index`

same

::: js/src/builtin/TypedObject.js
@@ +276,5 @@
> +  var fieldNames = DESCR_STRUCT_FIELD_NAMES(this.descr);
> +  for (var i = 0; i < fieldNames.length; i++) {
> +    if (fieldNames[i] === propName)
> +      return this.moveToFieldIndex(i);
> +  }

Seems like it would read better with indexOf.

  var index = fieldNames.indexOf(propName);
  if (index != -1)
    return this.moveToFieldIndex(index);

Comment 21

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

Comment on attachment 8370905 [details] [diff] [review]
Bug966575-Part07.diff

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

Clearing review for now pending better understanding of how this avoids moving GC.

::: js/src/jit-test/tests/TypedObject/jit-read-int.js
@@ +8,5 @@
>  function foo() {
>    for (var i = 0; i < 30000; i += 3) {
>      var pt = new PointType({x: i, y: i+1, z: i+2});
> +    var sum = pt.x + pt.y /* + pt.z */ ;
> +    //assertEq(sum, 3*i + 3);

?

::: js/src/jit/CodeGenerator.cpp
@@ +3124,5 @@
> +
> +        char *str = (char*) malloc(256);
> +        sprintf(str, "MIR instruction %d returned object with unexpected type",
> +                mir->id());
> +        masm.assumeUnreachable(str);

Bleagh! I guess it's fine.

::: js/src/jit/TypeDescrSet.cpp
@@ +67,5 @@
> +        return true;
> +    }
> +
> +    // Otherwise, use binary search to find the right place to insert
> +    // the type descriptor. We keep list sorted by the *address* of

keep *the list

So these are totally incompatible with moving GC. Is that documented somewhere? How do you enforce that TypeDescr's are pretenured (I haven't looked)? Can you assert that they're not in the nursery when adding them to this hash table?

Comment 22

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

Comment on attachment 8370905 [details] [diff] [review]
Bug966575-Part07.diff

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

Clearing review for now pending better understanding of how this avoids moving GC.

::: js/src/builtin/TypeRepresentation.cpp
@@ +232,5 @@
> +
> +size_t
> +ScalarTypeRepresentation::alignment(Type t)
> +{
> +    return ScalarSizes[t];

It looks like you do the same thing below, but just to confirm: alignment should be the same as the size here?

::: js/src/jit-test/tests/TypedObject/jit-read-int.js
@@ +8,5 @@
>  function foo() {
>    for (var i = 0; i < 30000; i += 3) {
>      var pt = new PointType({x: i, y: i+1, z: i+2});
> +    var sum = pt.x + pt.y /* + pt.z */ ;
> +    //assertEq(sum, 3*i + 3);

?

::: js/src/jit/CodeGenerator.cpp
@@ +3124,5 @@
> +
> +        char *str = (char*) malloc(256);
> +        sprintf(str, "MIR instruction %d returned object with unexpected type",
> +                mir->id());
> +        masm.assumeUnreachable(str);

Bleagh! I guess it's fine.

::: js/src/jit/TypeDescrSet.cpp
@@ +67,5 @@
> +        return true;
> +    }
> +
> +    // Otherwise, use binary search to find the right place to insert
> +    // the type descriptor. We keep list sorted by the *address* of

keep *the list

So these are totally incompatible with moving GC. Is that documented somewhere? How do you enforce that TypeDescr's are pretenured (I haven't looked)? Can you assert that they're not in the nursery when adding them to this hash table?

Comment 23

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

I think part 7 may need a rebase. I tried to apply it to figure out whether an include was necessary, and failed horribly.

Comment 24

[Niko Matsakis [:nmatsakis]]

(In reply to Steve Fink [:sfink] from comment #21)
> ::: js/src/jit-test/tests/TypedObject/jit-read-int.js
> @@ +8,5 @@
> >  function foo() {
> >    for (var i = 0; i < 30000; i += 3) {
> >      var pt = new PointType({x: i, y: i+1, z: i+2});
> > +    var sum = pt.x + pt.y /* + pt.z */ ;
> > +    //assertEq(sum, 3*i + 3);
> 
> ?

D'oh! I forgot about these changes, which were for debugging. I will revert.

> ::: js/src/jit/CodeGenerator.cpp
> @@ +3124,5 @@
> > +
> > +        char *str = (char*) malloc(256);
> > +        sprintf(str, "MIR instruction %d returned object with unexpected type",
> > +                mir->id());
> > +        masm.assumeUnreachable(str);
> 
> Bleagh! I guess it's fine.

No this is not fine, it's a memory leak. This was a debugging change I forgot to revert, sorry!

> ::: js/src/jit/TypeDescrSet.cpp
> @@ +67,5 @@
> > +        return true;
> > +    }
> > +
> > +    // Otherwise, use binary search to find the right place to insert
> > +    // the type descriptor. We keep list sorted by the *address* of
> 
> keep *the list
> 
> So these are totally incompatible with moving GC. Is that documented
> somewhere? How do you enforce that TypeDescr's are pretenured (I haven't
> looked)? Can you assert that they're not in the nursery when adding them to
> this hash table?

I believe this is OK because the GC cannot run during ion compilation. This is why ion in general uses raw pointers without worrying about moving GC. (Incidentally, I plan to remove this TypeDescrSet, per bug 933762, but obviously we wouldn't want to add invalid code in the meantime.)

Comment 25

[Niko Matsakis [:nmatsakis]]

> I'm attempting to follow along with the spec and what's happening here, but
> I can't say I really understand all this.

Argh, that reminds me. I wanted to note that, for the most part, none of these patches are intended to make any user-visible changes, with some small exceptions, where I was correcting deviations from the "spec". One challenge is that the spec is not fully written up yet.

The major exception I recall is that I removed the `fieldNames` property from type descriptors, which was an array with the names of all fields in the struct. This is not present in the struct and you don't really need it, since you can use `Object.getOwnPropertyNames(descr.fieldTypes)` if you prefer.

Comment 26

[Niko Matsakis [:nmatsakis]]

(In reply to Steve Fink [:sfink] from comment #18)
> Comment on attachment 8370896 [details] [diff] [review]
> Bug966575-Part03.diff
> 
> Review of attachment 8370896 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm attempting to follow along with the spec and what's happening here, but
> I can't say I really understand all this.
> 
> ::: js/src/builtin/TypedObject.js
> @@ +272,5 @@
> >           "moveToField invoked on non-struct");
> >    assert(HAS_PROPERTY(this.descr.fieldTypes, propName),
> >           "moveToField invoked with undefined field");
> >  
> > +  // FIXME -- opaque??
> 
> Bug #?

Sorry, this was a note to myself. It was referring to the fact that the `fieldOffsets` field on type descriptors, which we were using in this code, is only available on transparent types -- for opaque types (meaning those that contain references) the fieldOffsets is hidden from users. However, in one of the later patches, I resolve this FIXME by rewriting the code to use internal lists. I'll adjust the "FIXME" to (a) describe what the problem is and (b) indicate it is fixed in a later part of this bug.

Comment 27

[Niko Matsakis [:nmatsakis]]

(In reply to Steve Fink [:sfink] from comment #22)
> ::: js/src/builtin/TypeRepresentation.cpp
> @@ +232,5 @@
> > +
> > +size_t
> > +ScalarTypeRepresentation::alignment(Type t)
> > +{
> > +    return ScalarSizes[t];
> 
> It looks like you do the same thing below, but just to confirm: alignment
> should be the same as the size here?

Yes, for scalar types the alignment is always the same as the size.

Comment 28

[Niko Matsakis [:nmatsakis]]

(In reply to Niko Matsakis [:nmatsakis] from comment #24)
> I believe this is OK because the GC cannot run during ion compilation. This
> is why ion in general uses raw pointers without worrying about moving GC.
> (Incidentally, I plan to remove this TypeDescrSet, per bug 933762, but
> obviously we wouldn't want to add invalid code in the meantime.)

Re-reading your comment, it occurs to me that I may not understand how ion and the GGC interact. Perhaps it is only safe for ion to access tenured pointers directly? In that case, I would probably want to pre-tenure the type descriptors (which seems fine, I would in fact expect them to be long-lived).

Comment 29

[Niko Matsakis [:nmatsakis]]

From conversations in #ionmonkey, it seems that I do need to ensure that type descriptors are pre-tenured.

Comment 30

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part04.5.diff

Comment 31

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part06.5.diff

Comment 32

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part07.5.diff

Comment 33

[Niko Matsakis [:nmatsakis]]

sfink, I just uploaded 3 patches that together (1) pretenure the type descriptors and the various objects they can reach, (2) assert that type descriptors are pretenured when added into TI type objects and extracted for use in ion compilation.

Comment 34

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part02.diff

Rebased, carrying over r+ from sfink

Comment 35

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part03.diff

Rebased, carrying over r+ from sfink

Comment 36

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part04.diff

Rebased, carrying over r+ from sfink

Comment 37

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part05.diff

Rebased, carrying over r+ from sfink

Comment 38

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part06.diff

Rebased, carrying over r+ from sfink

Comment 39

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part07.diff

Removed the debugging code. Rerequesting review in light of the (separately posted) tenuring changes.

Comment 40

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part08.diff

Rebased, carrying over r+ from sfink

Comment 41

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part09.diff

Rebased, carrying over r+ from sfink

Comment 42

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part10.diff

Rebased, carrying over r+ from sfink

Comment 43

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part11.diff

Rebased, carrying over r+ from sfink

Comment 44

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part12.diff

Comment 45

[Niko Matsakis [:nmatsakis]]

Try run with all patches and bug 968866: https://tbpl.mozilla.org/?tree=Try&rev=8718ae71686d

Comment 46

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

Comment on attachment 8371567 [details] [diff] [review]
Bug966575-Part07.diff

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

::: js/src/jit/TypeDescrSet.cpp
@@ +68,5 @@
> +    }
> +
> +    // Otherwise, use binary search to find the right place to insert
> +    // the type descriptor. We keep the list sorted by the *address* of
> +    // the type representations within.

Did you mean to say "type *representations*" here?

Any place that is using the actual addresses of gcthings needs a comment saying why it's moving GC safe. I'm not 100% of what that explanation should be here, since I'm still chewing on the other patches. (It's either "because these are always pre-tenured" or "because the whole lifetime of this map is contained within an AutoSuppressGC for Ion compilation", I haven't figured out which.) But other than that, this patch is r=me.

Comment 47

[Niko Matsakis [:nmatsakis]]

(In reply to Steve Fink [:sfink] from comment #46)
> Any place that is using the actual addresses of gcthings needs a comment
> saying why it's moving GC safe.

I'll adjust the comment. Certainly I believe that everything within that list is pretenured (unless there is a bug in the other patches). I guess that makes it safe to rely on the address whether or not we are in Ion compilation? In this case we have the added security blanket of being in an ion compilation, and hence not overlapping a major GC. I am not sure of the precise invariants our GGC gives: that is, I don't know whether tenuring is enough to guarantee that things won't move during a major GC, but I think tenuring + ion compilation is certainly enough (and reasonably so).

Comment 48

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

Tenured stuff never moves during any sort of GC, major or minor. Ion compilation disables all GCs, both major and minor. I believe some of the ion machinery will get invoked at times when you *can* GC, eg when updating ICs.

mrgiggles knows when things are fully protected by an AutoSuppressGC.

  mrgiggles: can StructMetaTypeDescr::layout gc?
  sfink: Yes, |uint8 js::StructMetaTypeDescr::layout(JSContext*, class JS::Handle<js::StructTypeDescr*>, class JS::Handle<JSObject*>)| can GC: http://pastebin.mozilla.org/4208834

so eg StructMetaTypeDescr::layout *is* called within a call chain where GC is not suppressed.

My main hesitation with all of the pre-tenuring stuff is that it's a really big hammer. It would be better to nursery allocate as much as possible. If there are parts of the code that can only be called during ion compilation and create objects whose lifetime ends before the compilation does, then those parts can safely nursery allocate anything they want. I don't have a good mental model for what is guaranteed to be within the scope of ion compilation and what the lifetime of various things is. But it would be much better to not rely on pre-tenuring when possible. That's a slower path. And especially for stuff that can become garbage, it'll prevent compaction.

I'll take another look tomorrow and try to understand what's going on.

Comment 49

[Niko Matsakis [:nmatsakis]]

OK. We can also chat about it on IRC. I recognize that pretenuring is a more expensive allocation path but I think it's actually appropriate here: we are only talking about pretenuring the *type descriptors*, not the typed objects themselves, and I believe they are quite likely to be long-lived.

As I said in the bug intro, I don't think the TypeRepresentation objects are buying us much, and they introduce an asymmetry between the *spec* (which mentions only type descriptors) and the implementation (which has two kinds of objects). I originally envisioned that we would be able to ignore the TypeDescrs and just work with the TypeRepresentation objects, but it turns out that due to the nature of the API one must always track the original TypeDescr, because each user-defined type has its own prototype, even if they are structurally equivalent.

With this set of patches, the only thing they are used for is to determine when two TypeDescrs are structurally equivalent, which we can in turn use to do a memcpy rather than copying field by field. It should be easy to modify this hash-consing array so that it operates directly on the type descriptors, particularly as they are pre-tenured.

The reason I think pre-tenuring is appropriate is because the spec is not designed for users to create large number of type descriptors. They are not particularly lightweight. In fact, in the one case where we found users *were* inclined to create type descriptors every iteration (array types with fixed lengths), we have redesigned the API so that this is not convenient or necessary (though these changes haven't yet landed, I intend to implement them next).

The other option, I guess, is to keep the type representations around, even if only for the purposes of hash-consing and ion integration. They are also quite heavy weight -- they require a malloc and a pretenured object allocation -- but only for each unique type that is created. So it's *conceivable* that this might be somewhat faster if equivalent type descriptors were being created in a loop. But I think the gain would be relatively small.

Comment 50

[Niko Matsakis [:nmatsakis]]

After some discussion with terrence/sfink on IRC, my new plan to finish off the work in this bug is to replace the role of type representations in canonicalization by using atomized strings.

Comment 51

[Niko Matsakis [:nmatsakis]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef576b3af3f
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a68d8735fca
https://hg.mozilla.org/integration/mozilla-inbound/rev/182eee4ae305
https://hg.mozilla.org/integration/mozilla-inbound/rev/47b5dae4253d
https://hg.mozilla.org/integration/mozilla-inbound/rev/639a7d0a8404
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ff5470b1d28
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ad2702b533
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f126621f653
https://hg.mozilla.org/integration/mozilla-inbound/rev/e354941ec7c4
https://hg.mozilla.org/integration/mozilla-inbound/rev/22deb61f86cd
https://hg.mozilla.org/integration/mozilla-inbound/rev/22d628a02331
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0158d225a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c91be38b41c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b25714ee0717

Comment 52

[Niko Matsakis [:nmatsakis]]

Try run with all patches and bug 968866: https://tbpl.mozilla.org/?tree=Try&rev=8718ae71686d

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ef576b3af3f
https://hg.mozilla.org/mozilla-central/rev/3a68d8735fca
https://hg.mozilla.org/mozilla-central/rev/182eee4ae305
https://hg.mozilla.org/mozilla-central/rev/47b5dae4253d
https://hg.mozilla.org/mozilla-central/rev/639a7d0a8404
https://hg.mozilla.org/mozilla-central/rev/7ff5470b1d28
https://hg.mozilla.org/mozilla-central/rev/f1ad2702b533
https://hg.mozilla.org/mozilla-central/rev/5f126621f653
https://hg.mozilla.org/mozilla-central/rev/e354941ec7c4
https://hg.mozilla.org/mozilla-central/rev/22deb61f86cd
https://hg.mozilla.org/mozilla-central/rev/22d628a02331
https://hg.mozilla.org/mozilla-central/rev/ad0158d225a5
https://hg.mozilla.org/mozilla-central/rev/2c91be38b41c
https://hg.mozilla.org/mozilla-central/rev/b25714ee0717

Comment 54

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part13.diff

Comment 55

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part14.diff

Comment 56

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part15.diff

Comment 57

[Niko Matsakis [:nmatsakis]]

Comment on attachment 8380909 [details] [diff] [review]
Bug966575-Part15.diff

Didn't mean to attach this.

Comment 58

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

Comment on attachment 8380906 [details] [diff] [review]
Bug966575-Part13.diff

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

Nice!

Comment 59

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part13.diff

Rebased, carrying over r+

Comment 60

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part14.diff

Rebased, converted to use CheckedInt32, carrying over r+

Comment 61

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part14.diff

Decided that the CheckedInt32 conversion was best kept separate. Rebased, carrying over r+ from sfink.

Comment 62

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part15.diff

Converted to use CheckedInt32 rather than the hokey checks I was using before. This seems to have resolved some sporadic failures I was seeing but was never directly able to reproduce where we overflowed the int32 bounds.

Comment 63

[Niko Matsakis [:nmatsakis]]

Try run with just patch 13: https://tbpl.mozilla.org/?tree=Try&rev=86c258988b2f
Try run with patches 13-15: https://tbpl.mozilla.org/?tree=Try&rev=3b48e7e60c08

Both look mostly green, though there are some failures I can't clearly pin to an existing bug (but which do not seem immediately related). Digging a bit deeper.

Comment 64

[Jeff Walden [:Waldo]]

Comment on attachment 8392764 [details] [diff] [review]
Bug966575-Part15.diff

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

::: js/src/builtin/TypedObject.cpp
@@ +26,5 @@
>  
>  #include "vm/Shape-inl.h"
>  
>  using mozilla::DebugOnly;
> +using mozilla::CheckedInt32;

Alphabetical would put this before the other.

I would prefer |using mozilla::Checked;| and then using Checked<int32_t> in this file, myself.

@@ +74,2 @@
>      JS_ASSERT(IsPowerOfTwo(align));
> +    return ((address + align - 1) / align) * align;

So this takes an address, and produces the smallest address greater than or equal to it, that's aligned to |align|?  So unreadable.  :-)

I might rename this |roundUpToAlignment| for more clarity, but perhaps in a separate patch if this is used a bunch of places probably.

Division and multiplication are absolutely not going to be fast, ever.  I agree with getting rid of the negation, for clarity, but how about we do it like this instead:

    return (address + align - 1) & ~(align - 1);

Comment 65

[Niko Matsakis [:nmatsakis]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #64)
> I would prefer |using mozilla::Checked;| and then using Checked<int32_t> in
> this file, myself.

The actual name is CheckedInt, which means that uses look like CheckedInt<int32_t>. This seems rather repetitious, hence I assumed that the typedefs were the intended way to do things. Would you still prefer me to change it?

> I might rename this |roundUpToAlignment| for more clarity, but perhaps in a
> separate patch if this is used a bunch of places probably.

It's only used a few places, I will rename it.

> Division and multiplication are absolutely not going to be fast, ever. 

I don't especially care which definition we use (mul/div vs bitmasks). The code is never in any performance sensitive path in any case. I'll update it to use a bitmask just for fun.

Comment 66

[Niko Matsakis [:nmatsakis]]

(In reply to Niko Matsakis [:nmatsakis] from comment #65)
> > Division and multiplication are absolutely not going to be fast, ever. 
> 
> I don't especially care which definition we use (mul/div vs bitmasks). The
> code is never in any performance sensitive path in any case. I'll update it
> to use a bitmask just for fun.

Ah, right. It appears that the operator `&` is not defined on `CheckedInt`.

Comment 67

[Niko Matsakis [:nmatsakis]]

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

Comment 68

[Niko Matsakis [:nmatsakis]]

Attachment: Bug966575-Part15.diff

Nits addressed, r=waldo

Comment 69

[Niko Matsakis [:nmatsakis]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8306357eaea5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2f97d3cb75e9
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6ed5bf00b4

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8306357eaea5
https://hg.mozilla.org/mozilla-central/rev/2f97d3cb75e9
https://hg.mozilla.org/mozilla-central/rev/bb6ed5bf00b4

Comment 71

[Firefox Commit Bot]

The following changesets are now in Firefox Nightly:

> 8306357eaea5 Bug 966575 Part 13 -- Remove type repr completely, replacing with atomized strings r=sfink
> 2f97d3cb75e9 Bug 966575 Part 14 -- Convert from size_t to int32_t to reflect reality that we do not support objects larger than 2Gig r=sfink
> bb6ed5bf00b4 Bug 966575 Part 15 -- Port to use CheckedInt32 r=waldo

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central