1064358 - Scalar Replacement of Object should handle new functions.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Nicolas B. Pierron [:nbp]]

Attachment: 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.

Comment 1

[Nicolas B. Pierron [:nbp]]

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.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: Recover state of MCreateThisWithTemplate objects.

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.

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 8489496 [details] [diff] [review]
Recover state of MCreateThisWithTemplate objects.

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

Nice.

::: 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.

Comment 4

[Nicolas B. Pierron [:nbp]]

Attachment: Recover CreateThisWithTemplate.

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: Octane-raytrace, new escape analysis hits.

Comment 6

[Nicolas B. Pierron [:nbp]]

(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 7

[Jan de Mooij [:jandem]]

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.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: Recover CreateThisWithTemplate.

Delta:
 - Use JSObject::create in JSObject::copy
 - Add a test case.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8490952 [details] [diff] [review]
Recover CreateThisWithTemplate.

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

Thanks.

Comment 10

[Nicolas B. Pierron [:nbp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fede9a580964
https://hg.mozilla.org/integration/mozilla-inbound/rev/e22df17b1de9

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fede9a580964
https://hg.mozilla.org/mozilla-central/rev/e22df17b1de9