1871147 - Don't emit code to run extra initializers unless decorators are present

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Dan Minor [:dminor]]

This is a follow up to Bug 1868461. In that bug, we avoid emitting code for the addInitializer function if decorators are not present. We can do better, and not emit the code to run extra initializers installed by decorators if decorators are not present.

This will require changing the BytecodeEmitter::emitInitializeInstanceMembers method at [1] to only emit the bytecode if decorators are present. I think we'll have to change the MemberInitializers structure to track this information, it's defined at [2].

[1] https://searchfox.org/mozilla-central/rev/3bd65516eb9b3a9568806d846ba8c81a9402a885/js/src/frontend/BytecodeEmitter.cpp#10648
[2] https://searchfox.org/mozilla-central/rev/3bd65516eb9b3a9568806d846ba8c81a9402a885/js/src/vm/SharedStencil.h#790

Comment 1

[Debadree Chatterjee]

I am ready to take this on next! My only question is where exactly does MemberInitializers get initialised i see the function findMemberInitializersForCall at [1] which I see is walking though the object and its parents to find out the member initialiser just teh question is where do these get initialised then. Also do we have to consider that any decorators either on the class as well as on the instance methods have to considered to setting hasDecorators to true?

[1] https://searchfox.org/mozilla-central/source/js/src/frontend/BytecodeEmitter.cpp#10540

Thank you for your time!

Regards

Comment 2

[Dan Minor [:dminor]]

Thank you for having a look at this one :)

I think maybe we don't need to worry about MemberInitializers after all, and we can just add another boolean argument to BytecodeEmitter::emitInitializeInstanceMembers for hasDecorators. You can hopefully set this the same way you did for the last bug, by checking if addInitializers is present for the class.

For now, we don't need to worry about class decorators, just instance decorators. This will change when Bug 1868221 is implemented, but right now, there's no way for class decorators to add extra initializers.

I will be away after today until January 8th, but I think Arai will be still be working if you have any questions or need someone for a code review.

Good luck! I really appreciate your help on these bugs.

Comment 3

[Debadree Chatterjee]

Hey thank you very much for the response, I am implementing this soon enough! and loving the learning I am getting will try to ramp up contributing here

Also happy holidays! have a lovely vacation!

Comment 4

[Debadree Chatterjee]

Hello!

Happy new year!
so sorry i was unfortunately unable to take look at this for these few days taking a look at it now again, I have a few questions

So when trying to add extra param for hasDecorators for BytecodeEmitter::emitInitializeInstanceMembers I am facing the issue on trying to find out whether decorators exist or not, I tried this following simple script

function dec1() {
  console.log('dec1')
}

class C {
  @dec1 x1 = 0;
}

let c = new C();

and traced that BytecodeEmitter::emitInitializeInstanceMembers is called from [1] now it seems unlikely that we can extract whether decorators from the funbox_ variable that we are using in the function, is there anything i am missing here? maybe we have to pass whether decorators exists from somewhere higher in the call stack?

Thank You!

[1] https://searchfox.org/mozilla-central/source/js/src/frontend/FunctionEmitter.cpp#381

Comment 5

[Dan Minor [:dminor]]

Attachment: WIP: Bug 1871147 - Don't emit code to run extra initializers unless decorators are present

Comment 6

[Dan Minor [:dminor]]

Happy New Year!

I'm really sorry it took so long for me to get back to you.

You're right, I don't think there's an easy way to add another boolean to ByteCodeEmitter::emitInitializeInstanceMembers so I think we're stuck adding a field to the MemberInitializers structure. I've attached a quick proof of concept to help get you started. It's not complete, and it needs testing, etc., but I think it's the correct approach to take here.

Comment 7

[Debadree Chatterjee]

Hello!

I am sorry for taking so long on this just one question, how could we test that the code wasn't emitted for extra initialisers, If i understand correctly here the extra initialisers are the functions which are added by calling context.addInitializer ? if no decorators are present any way to check from js whether the code was emitted?

Thank you!

Comment 8

[Dan Minor [:dminor]]

No worries, there's no rush :)

Sorry it took me so long to reply, I have been travelling and away from my computer.

You can do a simple test with a script like this:

function dec() {}

class C {
    @dec x = 2;
}

dis(C)

If you run this in the JavaScript shell, it will disassemble the code generated to run the initializers, you'll see stuff like this with decorators enabled:

# from Goto @ 00152
00038:   3  LoopHead (ic: 3, depthHint: 1) # .initializers .initializers.length 1
00044:   3  Dup                         # .initializers .initializers.length 1 1
00045:   3  DupAt 2                     # .initializers .initializers.length 1 1 .initializers.length
00049:   3  Lt                          # .initializers .initializers.length 1 (1 < .initializers.length)
00050:   3  JumpIfFalse 157 (+107)      # .initializers .initializers.length 1
00055:   3  JumpTarget (ic: 5)          # .initializers .initializers.length 1
00060:   3  DupAt 2                     # .initializers .initializers.length 1 .initializers

The goal is to stop generating this code if there are no decorators.

I'm not sure offhand if we can write a formal testcase for this, but hopefully this is enough to get you started.

Thanks!

Comment 9

[Debadree Chatterjee]

Attachment: Bug 1871147 - Omit extra initializers code unless decorators are present. r?dminor

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b22e4d52944a
Omit extra initializers code unless decorators are present. r=dminor

Comment 11

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/b22e4d52944a