Rewrite and optimize object allocation paths

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Created attachment 8837126 [details] [diff] [review]
Patch

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]
Patch

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

::: js/src/builtin/TypedObject.cpp
@@ +2400,5 @@
> +    // GetReservedOrProxyPrivateSlot.
> +    MOZ_ASSERT(JSCLASS_RESERVED_SLOTS(clasp) == 0);
> +    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)
(Assignee)

Comment 2

2 years ago
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.
(Assignee)

Comment 3

2 years ago
Created attachment 8837183 [details] [diff] [review]
Patch v2
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]:
-----------------------------------------------------------------

Thanks!
Attachment #8837183 - Flags: review?(bhackett1024) → review+

Comment 5

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/12667d6bc208
Rewrite and optimize object allocation paths. r=bhackett
(Assignee)

Updated

2 years ago
Blocks: 1339507

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/12667d6bc208
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54

Updated

2 years ago
Depends on: 1342170
You need to log in before you can comment on or make changes to this bug.