Closed
Bug 966575
Opened 11 years ago
Closed 11 years ago
Remove most uses of TypeRepresentation and replace with TypeDescr
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: nmatsakis, Assigned: nmatsakis)
References
Details
Attachments
(18 files, 16 obsolete files)
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
nmatsakis
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•11 years ago
|
Blocks: harmony:typedobjects
Assignee | ||
Comment 1•11 years ago
|
||
- 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.
Attachment #8369072 -
Flags: review?(sphink)
Comment 2•11 years ago
|
||
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
Attachment #8369072 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 3•11 years ago
|
||
Marking as [leave-open] since I did this work in many steps and I plan to land them one by one.
Whiteboard: [leave-open]
Assignee | ||
Comment 4•11 years ago
|
||
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.
Attachment #8370283 -
Flags: review?(sphink)
Assignee | ||
Comment 5•11 years ago
|
||
Try run for part 1: https://tbpl.mozilla.org/?tree=Try&rev=1f6f1f08d032
Assignee | ||
Comment 6•11 years ago
|
||
Comment 7•11 years ago
|
||
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #8370896 -
Flags: review?(sphink)
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #8370900 -
Flags: review?(sphink)
Assignee | ||
Comment 10•11 years ago
|
||
Attachment #8370901 -
Flags: review?(sphink)
Assignee | ||
Comment 11•11 years ago
|
||
Attachment #8370902 -
Flags: review?(sphink)
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8370905 -
Flags: review?(sphink)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8370906 -
Flags: review?(sphink)
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #8370908 -
Flags: review?(sphink)
Assignee | ||
Comment 15•11 years ago
|
||
Attachment #8370909 -
Flags: review?(sphink)
Assignee | ||
Comment 16•11 years ago
|
||
Attachment #8370910 -
Flags: review?(sphink)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #8370928 -
Flags: review?(sphink)
Updated•11 years ago
|
Attachment #8370283 -
Flags: review?(sphink) → review+
Comment 18•11 years ago
|
||
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 #?
Attachment #8370896 -
Flags: review?(sphink) → review+
Comment 19•11 years ago
|
||
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.)
Attachment #8370900 -
Flags: review?(sphink) → review+
Updated•11 years ago
|
Attachment #8370901 -
Flags: review?(sphink) → review+
Comment 20•11 years ago
|
||
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);
Attachment #8370902 -
Flags: review?(sphink) → review+
Comment 21•11 years ago
|
||
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?
Attachment #8370905 -
Flags: review?(sphink)
Comment 22•11 years ago
|
||
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?
Updated•11 years ago
|
Attachment #8370906 -
Flags: review?(sphink) → review+
Updated•11 years ago
|
Attachment #8370908 -
Flags: review?(sphink) → review+
Updated•11 years ago
|
Attachment #8370909 -
Flags: review?(sphink) → review+
Updated•11 years ago
|
Attachment #8370910 -
Flags: review?(sphink) → review+
Updated•11 years ago
|
Attachment #8370928 -
Flags: review?(sphink) → review+
Comment 23•11 years ago
|
||
I think part 7 may need a rebase. I tried to apply it to figure out whether an include was necessary, and failed horribly.
Assignee | ||
Comment 24•11 years ago
|
||
(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.)
Assignee | ||
Comment 25•11 years ago
|
||
> 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.
Assignee | ||
Comment 26•11 years ago
|
||
(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.
Assignee | ||
Comment 27•11 years ago
|
||
(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.
Assignee | ||
Comment 28•11 years ago
|
||
(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).
Assignee | ||
Comment 29•11 years ago
|
||
From conversations in #ionmonkey, it seems that I do need to ensure that type descriptors are pre-tenured.
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #8371554 -
Flags: review?(sphink)
Assignee | ||
Comment 31•11 years ago
|
||
Attachment #8371555 -
Flags: review?(sphink)
Assignee | ||
Comment 32•11 years ago
|
||
Attachment #8371557 -
Flags: review?(sphink)
Assignee | ||
Comment 33•11 years ago
|
||
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.
Assignee | ||
Comment 34•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8370283 -
Attachment is obsolete: true
Attachment #8371561 -
Flags: review+
Assignee | ||
Comment 35•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8370896 -
Attachment is obsolete: true
Attachment #8371562 -
Flags: review+
Assignee | ||
Comment 36•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8370900 -
Attachment is obsolete: true
Attachment #8371563 -
Flags: review+
Assignee | ||
Comment 37•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8370901 -
Attachment is obsolete: true
Attachment #8371564 -
Flags: review+
Assignee | ||
Comment 38•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8370902 -
Attachment is obsolete: true
Attachment #8371565 -
Flags: review+
Assignee | ||
Comment 39•11 years ago
|
||
Removed the debugging code. Rerequesting review in light of the (separately posted) tenuring changes.
Attachment #8370905 -
Attachment is obsolete: true
Attachment #8371567 -
Flags: review?(sphink)
Assignee | ||
Comment 40•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8371568 -
Flags: review+
Assignee | ||
Comment 41•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8371569 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8370906 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8370908 -
Attachment is obsolete: true
Assignee | ||
Comment 42•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8371571 -
Flags: review+
Assignee | ||
Comment 43•11 years ago
|
||
Rebased, carrying over r+ from sfink
Attachment #8370909 -
Attachment is obsolete: true
Attachment #8370910 -
Attachment is obsolete: true
Attachment #8371572 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → nmatsakis
Assignee | ||
Comment 44•11 years ago
|
||
Attachment #8371573 -
Flags: review+
Assignee | ||
Updated•11 years ago
|
Attachment #8370928 -
Attachment is obsolete: true
Assignee | ||
Comment 45•11 years ago
|
||
Try run with all patches and bug 968866: https://tbpl.mozilla.org/?tree=Try&rev=8718ae71686d
Comment 46•11 years ago
|
||
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.
Attachment #8371567 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 47•11 years ago
|
||
(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•11 years ago
|
||
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.
Assignee | ||
Comment 49•11 years ago
|
||
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.
Assignee | ||
Comment 50•11 years ago
|
||
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.
Updated•11 years ago
|
Attachment #8371554 -
Flags: review?(sphink) → review+
Updated•11 years ago
|
Attachment #8371555 -
Flags: review?(sphink) → review+
Updated•11 years ago
|
Attachment #8371557 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 51•11 years ago
|
||
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
Assignee | ||
Comment 52•11 years ago
|
||
Try run with all patches and bug 968866: https://tbpl.mozilla.org/?tree=Try&rev=8718ae71686d
Comment 53•11 years ago
|
||
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
Assignee | ||
Comment 54•11 years ago
|
||
Attachment #8380906 -
Flags: review?(sphink)
Assignee | ||
Comment 55•11 years ago
|
||
Attachment #8380907 -
Flags: review?(sphink)
Assignee | ||
Comment 56•11 years ago
|
||
Attachment #8380909 -
Flags: review?(sphink)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave-open]
Assignee | ||
Comment 57•11 years ago
|
||
Comment on attachment 8380909 [details] [diff] [review]
Bug966575-Part15.diff
Didn't mean to attach this.
Attachment #8380909 -
Attachment is obsolete: true
Attachment #8380909 -
Flags: review?(sphink)
Comment 58•11 years ago
|
||
Comment on attachment 8380906 [details] [diff] [review]
Bug966575-Part13.diff
Review of attachment 8380906 [details] [diff] [review]:
-----------------------------------------------------------------
Nice!
Attachment #8380906 -
Flags: review?(sphink) → review+
Updated•11 years ago
|
Attachment #8380907 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 59•11 years ago
|
||
Rebased, carrying over r+
Attachment #8380906 -
Attachment is obsolete: true
Attachment #8392754 -
Flags: review+
Assignee | ||
Comment 60•11 years ago
|
||
Rebased, converted to use CheckedInt32, carrying over r+
Attachment #8380907 -
Attachment is obsolete: true
Attachment #8392755 -
Flags: review+
Assignee | ||
Comment 61•11 years ago
|
||
Decided that the CheckedInt32 conversion was best kept separate. Rebased, carrying over r+ from sfink.
Attachment #8392755 -
Attachment is obsolete: true
Attachment #8392762 -
Flags: review+
Assignee | ||
Comment 62•11 years ago
|
||
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.
Attachment #8392764 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 63•11 years ago
|
||
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•11 years ago
|
||
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);
Attachment #8392764 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 65•11 years ago
|
||
(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.
Assignee | ||
Comment 66•11 years ago
|
||
(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`.
Assignee | ||
Comment 67•11 years ago
|
||
Assignee | ||
Comment 68•11 years ago
|
||
Nits addressed, r=waldo
Attachment #8392764 -
Attachment is obsolete: true
Attachment #8399948 -
Flags: review+
Assignee | ||
Comment 69•11 years ago
|
||
Comment 70•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8306357eaea5
https://hg.mozilla.org/mozilla-central/rev/2f97d3cb75e9
https://hg.mozilla.org/mozilla-central/rev/bb6ed5bf00b4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Comment 71•11 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•