Closed
Bug 1339411
Opened 7 years ago
Closed 7 years ago
Rewrite and optimize object allocation paths
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jandem, Assigned: jandem)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
37.99 KB,
patch
|
bhackett1024
:
review+
|
Details | Diff | 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 1•7 years ago
|
||
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•7 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•7 years ago
|
||
Attachment #8837126 -
Attachment is obsolete: true
Attachment #8837183 -
Flags: review?(bhackett1024)
Comment 4•7 years ago
|
||
Comment on attachment 8837183 [details] [diff] [review] Patch v2 Review of attachment 8837183 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8837183 -
Flags: review?(bhackett1024) → review+
Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/12667d6bc208 Rewrite and optimize object allocation paths. r=bhackett
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/12667d6bc208
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•