Closed Bug 1339411 Opened 7 years ago Closed 7 years ago

Rewrite and optimize object allocation paths


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox54 --- fixed


(Reporter: jandem, Assigned: jandem)


(Blocks 1 open bug)



(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
Right now all of our VM object allocations go through NewObject* and then JSObject::create.

Problem is that the different object types (for example unboxed objects and proxies) have very little in common, so there are a ton of branches to handle special cases that are only relevant for some objects, and this makes the code pretty slow and hard to reason about.

The attached patch splits JSObject::create in:

* NativeObject::create
* ProxyObject::create
* TypedObject::create
* UnboxedObject::createInternal (to distinguish from Unboxed*Object::create)

Furthermore, proxies and unboxed objects no longer go through the NewObject* machinery at all, it's simpler and faster to inline the few bits we actually needed from that. (This also means when we allocate an unboxed object, we no longer look up an initial shape that we don't use.)

This patch already is a measurable win on a wrapper allocation micro-benchmark, but the next step is optimizing ProxyObject::create more (Google Docs creates a ton of wrappers) by caching the shape and group. Doing that will be much simpler on top of this patch.

Finally, this should also make it easier to experiment with different proxy layouts in the future, bug 1237504.
Attachment #8837126 - Flags: review?(bhackett1024)
Comment on attachment 8837126 [details] [diff] [review]

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

::: js/src/builtin/TypedObject.cpp
@@ +2400,5 @@
> +    // GetReservedOrProxyPrivateSlot.
> +    MOZ_ASSERT(!clasp->hasPrivate());
> +    MOZ_ASSERT(shape->numFixedSlots() == 0);
> +    MOZ_ASSERT(shape->slotSpan() == 0);

I like the idea and speed and cleanliness improvements here, but a bunch of these assertions are now being duplicated in multiple places with subtle variations that tie into engine wide invariants we expect to hold, like the thing for GetReservedOrProxyPrivateSlot.

Can you expand debugCheckFinalizeFlags into something like debugCheckNewObject, which takes the group, shape, and other bits and asserts all the properties we expect to hold for all objects in the engine?
Attachment #8837126 - Flags: review?(bhackett1024)
Sure, that's a fair suggestion. I was thinking not all of the asserts would be relevant for each of the new allocation sites, but yeah maybe better to have a consistent set of checks. I'll update the patch.
Attached patch Patch v2Splinter Review
Attachment #8837126 - Attachment is obsolete: true
Attachment #8837183 - Flags: review?(bhackett1024)
Comment on attachment 8837183 [details] [diff] [review]
Patch v2

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

Attachment #8837183 - Flags: review?(bhackett1024) → review+
Pushed by
Rewrite and optimize object allocation paths. r=bhackett
Blocks: 1339507
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Depends on: 1342170
Regressions: 1541580
You need to log in before you can comment on or make changes to this bug.