Last Comment Bug 684507 - Remove JSObject::isNewborn()
: Remove JSObject::isNewborn()
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
-- normal with 1 vote (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
Depends on:
Blocks: ObjectShrink
  Show dependency treegraph
Reported: 2011-09-03 08:49 PDT by Brian Hackett (:bhackett)
Modified: 2011-12-07 16:44 PST (History)
5 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (96.30 KB, patch)
2011-11-02 13:35 PDT, Brian Hackett (:bhackett)
luke: review+
Details | Diff | Splinter Review

Description User image Brian Hackett (:bhackett) 2011-09-03 08:49:11 PDT
Every time the GC marks an object it first needs to check isNewborn() (lastProp != NULL), which if false means the rest of the object is garbage.  This is because places where objects are created can trigger a GC after having allocated the object but before it is filled in, and these places are required to fill in lastProp last.  It would be simpler and faster to require these places to assemble the contents of the object (shape, type, parent) first, allocate the object and fill it in entirely without any potential for a GC when it is half-initialized.
Comment 1 User image Brian Hackett (:bhackett) 2011-11-02 13:35:00 PDT
Created attachment 571437 [details] [diff] [review]

Restructure object creation so that the type, shape, and necessary slots are allocated up front and the object is filled to a valid state immediately after creation.  This is basically a rewrite of the object creation code.  Luke, sorry for saddling you with all these object shrink reviews, let me know if you want me to hand any off to other folks.

Background: the creation path cleanup is necessary because creating objects in the VM is currently much slower in the JM branch than on trunk due to the various hashtable lookups involved.  This funnels things better and will allow making the VM creation path fast again (faster than trunk) through the use of a couple caches.
Comment 2 User image Luke Wagner [:luke] 2011-11-16 09:37:34 PST
Comment on attachment 571437 [details] [diff] [review]

Review of attachment 571437 [details] [diff] [review]:

A most righteous patch!

::: js/src/jsfun.cpp
@@ +155,5 @@
> +    JS_STATIC_ASSERT(StrictArgumentsObject::RESERVED_SLOTS == 2);
> +    JSObject *obj = js_NewGCObject(cx, FINALIZE_OBJECT4);
> +    if (!obj)
> +        return NULL;
> +    obj->initialize(emptyArgumentsShape, type, NULL);

Further reaping the rewards of your work: could you make a static JSObject::create to factor out the js_NewGCObject+initialize pattern (thereby allowing your to remove initialize())?

::: js/src/jsobjinlines.h
@@ +937,5 @@
> +        setPrivate(NULL);
> +
> +    size_t span = shape->slotSpan();
> +    if (span)
> +        clearSlotRange(0, span);

if (size_t span = ...)

@@ +1604,3 @@
>  {
> +    size_t count = JSObject::dynamicSlotsCount(shape->numFixedSlots(), shape->slotSpan());
> +    if (count) {

if (size_t count = ...)

Also, can we name this AllocateObjectDynamicSlots?

Note You need to log in before you can comment on or make changes to this bug.