When comparing two simple microbenchmarks that construct and then repeatedly GC a deep binary tree, if the tree is composed of typed objects the GC takes nearly twice as long as if it is composed of normal objects. This is due to the overhead of typed object tracing, which goes through the entire type descriptor looking for references which need to be traced. This is way slower than the highly optimized paths for marking native objects. The attached patch optimizes GC marking code for inline typed objects, whose only outgoing references are references in their data (plus references in the lazy buffer table, though those are taken care of by the weak map itself). When constructing a type descriptor which is small enough to fit in an inline typed object, we traverse its contents and construct a simple array of offsets indicating which things in the descriptor need to be marked. This array is then used during GC marking. This speeds up marking of the binary tree enough that the typed objects version can be collected slightly faster than the normal objects version.

Comment on attachment 8523576 [details] [diff] [review] patch Review of attachment 8523576 [details] [diff] [review]: ----------------------------------------------------------------- I very much like the functionality, but the code duplication is too much for me to r+ without some serious convincing. If you break out value and string marking into static inlines (where the value one calls the string one) and call them from ProcessMarkStackTop, does it run slower? ::: js/src/builtin/TypedObject.cpp @@ +3214,5 @@ > +{ > + for (size_t i = 0; i < stringOffsets.length(); i++) { > + if (!entries.append(stringOffsets[i])) > + return false; > + } Can you use entries.appendAll(stringOffsets) here? @@ +3239,5 @@ > + // Trace lists are only used for inline typed objects. We don't use them > + // for larger objects, both to limit the size of the trace lists and > + // because tracing outline typed objects is considerably more complicated > + // than inline ones. > + if ((size_t) descr->size() > InlineTypedObject::MaximumSize || descr->transparent()) { This is a scary connection. Here you check descr->size() against a max value. During actual tracing, you just check for the obj_trace function pointer. If these don't match, you'll end up not tracing something. size() > MaximumSize is meant to be exactly equivalent to this being an outline object. At the very least, move this size check into a named utility function and use it both here and createZeroed. But I think it would also be good to assert somewhere that if an object passes this test, it will fail the obj_trace == InlineTypedObject::obj_trace test. @@ +3253,5 @@ > + return false; > + > + // Don't create a trace list for descriptors with no references. > + MOZ_ASSERT(entries.length() >= 3); > + if (entries.length() == 3) { I would not describe this as "Don't create a trace list", since you will be running the specialized trace code no matter whether the list is null or not. It's more something like "Use nullptr to signify that descriptors with no references do not need to be traced." Or just "a nullptr is interpreted as an empty trace list." ::: js/src/builtin/TypedObject.h @@ +199,5 @@ > + // Type descriptors may contain a list of their references for use during > + // scanning. Marking code is optimized to use this list, rather than the > + // slower trace hook. This list is only specified when (a) the descriptor > + // is short enough that it can fit in an InlineTypedObject, and (b) the > + // descriptor contains at least one reference. Otherwise it is null. These have two different semantics for the nullptr, don't they? In (a) it will never be used so it is set to null. (Shouldn't it be poisoned or something in debug builds?) In (b) the nullptr is an explicit signal that nothing will be traced, equivalent to an empty list.

(In reply to Steve Fink [:sfink, :s:] (PTO til Dec 1) from comment #1) > @@ +3239,5 @@ > > + // Trace lists are only used for inline typed objects. We don't use them > > + // for larger objects, both to limit the size of the trace lists and > > + // because tracing outline typed objects is considerably more complicated > > + // than inline ones. > > + if ((size_t) descr->size() > InlineTypedObject::MaximumSize || descr->transparent()) { > > This is a scary connection. Here you check descr->size() against a max > value. During actual tracing, you just check for the obj_trace function > pointer. If these don't match, you'll end up not tracing something. > > size() > MaximumSize is meant to be exactly equivalent to this being an > outline object. At the very least, move this size check into a named utility > function and use it both here and createZeroed. But I think it would also be > good to assert somewhere that if an object passes this test, it will fail > the obj_trace == InlineTypedObject::obj_trace test. Eh, we need to check the size here because this is the code where we create type descriptors, not typed objects. A descriptor that fits in MaximumSize can be used by both inline and outline typed objects, whereas larger descriptors can only be used by outline typed objects. I don't see how using a new utility function will help here, as using the MaximumSize constant in both places is also tying them together. If we were to create inline typed objects with descriptors larger than MaximumSize we would have have some serious problems unrelated to tracing. > ::: js/src/builtin/TypedObject.h > @@ +199,5 @@ > > + // Type descriptors may contain a list of their references for use during > > + // scanning. Marking code is optimized to use this list, rather than the > > + // slower trace hook. This list is only specified when (a) the descriptor > > + // is short enough that it can fit in an InlineTypedObject, and (b) the > > + // descriptor contains at least one reference. Otherwise it is null. > > These have two different semantics for the nullptr, don't they? In (a) it > will never be used so it is set to null. (Shouldn't it be poisoned or > something in debug builds?) In (b) the nullptr is an explicit signal that > nothing will be traced, equivalent to an empty list. With (a) the trace list will still be accessed by the finalizer, but otherwise, yeah, it could be poisoned.

Updated patch per comments. I don't see a difference between this and the earlier one in tracing times on 10.9 x64.