Closed Bug 1064358 Opened 6 years ago Closed 6 years ago

Scalar Replacement of Object should handle new functions.


(Core :: JavaScript Engine: JIT, defect)

Not set





(Reporter: nbp, Assigned: nbp)




(4 files, 1 obsolete file)

Attached file ea-newfun.js
To handle non-escaped objects which are created as such:

function Point(x, y) {
  this.x = x;
  this.y = y;

new Point(1,2);

we need to add support for MCreateThisWithTemplate and replace MSetPropertyCache by a variant (MAddProperty maybe?) which can be optimized out by Scalar Replacement phase.
Blocks: 1014649
No longer depends on: 1014649
Depends on: 1064362
Depends on: 1067489
attachment 8485780 [details] is ~10% faster by only working around Bug 1067489 and without recovering the allocation of MCreateThisWithTemplate.

Note: If we were able to replace "(x+1) - x" by "1", then this would be a micro-benchmarks where only the assertEq calls remains.
Assignee: nobody → nicolas.b.pierron
A second patch is coming for removing the allocation of
MCreateThisWithTemplate.  Currently this patch is not optimizing anything as
it is blocked by Bug 1067489.
Attachment #8489496 - Flags: review?(jdemooij)
Comment on attachment 8489496 [details] [diff] [review]
Recover state of MCreateThisWithTemplate objects.

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


::: js/src/jit/MIR.cpp
@@ +3013,5 @@
> +    JSObject *templateObject = nullptr;
> +    if (obj->isNewObject())
> +        templateObject = obj->toNewObject()->templateObject();
> +    else if (obj->isCreateThisWithTemplate())
> +        templateObject = obj->toCreateThisWithTemplate()->templateObject();

Nit: you could just turn it into an |else|, the toCreateThiswithTemplate() call will also assert if it's something else.
Attachment #8489496 - Flags: review?(jdemooij) → review+
Attached patch Recover CreateThisWithTemplate. (obsolete) — Splinter Review
Attachment #8490258 - Flags: review?(jdemooij)
(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> Created attachment 8490263 [details]
> Octane-raytrace, new escape analysis hits.

After checking octane over night (~720 runs),
I compared "--ion-scalar-replacement=off"  vs.  "--ion-scalar-replacement=on":

   RayTrace:   96937 ->  103874  7.2%
Comment on attachment 8490258 [details] [diff] [review]
Recover CreateThisWithTemplate.

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

\o/ Nice win! Looks great, just one comment below. If you can post an updated patch I can review it quickly.

::: js/src/jsobjinlines.h
@@ +572,5 @@
> +    MOZ_ASSERT(!templateObject->denseElementsAreCopyOnWrite());
> +    obj->elements = js::emptyObjectElements;
> +
> +    if (clasp->hasPrivate())
> +        obj->privateRef(shape->numFixedSlots()) = nullptr;

This function duplicates some code in JSObject::create and it's only used on the recover path so bugs could go unnoticed for a while.

Can copy() just call create() and then initialize the slots? I think create() will initialize the slots to undefined, but this shouldn't matter on the recover path.
Attachment #8490258 - Flags: review?(jdemooij)
 - Use JSObject::create in JSObject::copy
 - Add a test case.
Attachment #8490258 - Attachment is obsolete: true
Attachment #8490952 - Flags: review?(jdemooij)
Comment on attachment 8490952 [details] [diff] [review]
Recover CreateThisWithTemplate.

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

Attachment #8490952 - Flags: review?(jdemooij) → review+
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.