1405457 - Unused NewCallObject is not removed

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Tom S [:evilpie]]

On arrow-declare.es6 we remove the lambda with the help of scalar replacement, but somehow an unused NewCallObject is kept alive.

Per nbp: MNewCallObject is not recovered on bailout.

Comment 1

[Tom S [:evilpie]]

Attachment: testcase

Comment 2

[Ted Campbell [:tcampbell]]

For posterity:

My concern is to double check this doesn't screw up bailout handling of prologues such as https://searchfox.org/mozilla-central/source/js/src/jit/BaselineBailouts.cpp#731-742

:nbp reassures me that the recovery should recreate this before InitFromBailouts runs and so this isn't a problem.

Comment 3

[Tom S [:evilpie]]

Attachment: WIP: Scalar replacement for call objects

Comment 4

[Tom S [:evilpie]]

This passes tests locally \o/. I am not sure if we can CallObject::create instead of NewCallObject, we are not really running in ion while recovering, so the barriers don't matter?

Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4f323517d053e38952a5340f98a93a69b5e0351

From my past experience testing recover instructions is always a bit annoying. I did verify that at least some tests end up in RNewCallObject::recover.

Comment 5

[Tom S [:evilpie]]

Attachment: Scalar replacement for call objects

I just added a functional test based on six-speed benchmark. Continue using NewCallObject based on Jon's comments on IRC.

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8915119 [details] [diff] [review]
Scalar replacement for call objects

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

::: js/src/jit/MIR.h
@@ +13149,5 @@
>  
> +class MNewCallObjectBase : public MUnaryInstruction
> +                         , public SingleObjectPolicy::Data
> +{
> +  CompilerGCPointer<CallObject*> templateObj_;

nit: indent by 4.

Note: This is not be necessary as the MConstant would already do this, and you can always reach out to the templateObject pointer as done in MNewObject:

http://searchfox.org/mozilla-central/rev/a4702203522745baff21e519035b6c946b7d710d/js/src/jit/MIR.h#3582-3584

@@ +13204,5 @@
>        : MNewCallObjectBase(classOpcode, templateObj)
>      {}
>  
>      static MNewSingletonCallObject*
> +    New(TempAllocator& alloc, MConstant* templateObj)

nit: Use TRIVIAL_NEW_WRAPPERS macro. (do the same for MNewCallObject)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/53006df129c8
Scalar replacement for call objects. r=nbp

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/53006df129c8