1662915 - Refactor InitFromBailout

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, task, P2)

Description

[Iain Ireland [:iain]]

InitFromBailout is an 800+ line static function with at least three out-parameters. It is past time to break it up into something a little more structured.

Comment 1

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 1: Change BaselineStackBuilder from struct to class r=jandem

Deleting some dead code.

Comment 2

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 2: Add BaselineStackBuilder::initFrame r=jandem

My approach in this stack is to incrementally extract parts of InitFromBailout (and some parts of BailoutIonToBaseline into methods on BaselineStackBuilder. Some of the intermediate patches look a little strange, but I think it ends up in a reasonable place.

Depends on D89510

Comment 3

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 3: Add BaselineStackBuilder::buildBaselineFrame r=jandem

Depends on D89511

Comment 4

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 4: Add BaselineStackBuilder::nextFrame r=jandem

This will be expanded in a future patch to also update script_ and fun_.

Depends on D89512

Comment 5

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 5: Add BaselineStackBuilder::buildArguments r=jandem

I would like to add a comment here explaining why we have to overwrite the arguments in the caller, but I'm not confident I understand myself.

Similarly, I tried rewriting the comment about not updating when the arguments object aliases the formals ("In such cases, the list of arguments reported by the snapshot are only aliases of argument object slots..."), but I wasn't sure I understood.

Depends on D89513

Comment 6

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 6: Add BaselineStackBuilder::buildFixedSlots r=jandem

Depends on D89514

Comment 7

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 7: Add BaselineStackBuilder::fixUpCallerArgs r=jandem

To simplify things, I changed FunCall so that the arguments are pushed during fixup, instead of using the main expression stack slot loop (https://searchfox.org/mozilla-central/rev/84922363f4014eae684aabc4f1d06380066494c5/js/src/jit/BaselineBailouts.cpp#981-1019). This is safe because the special cases in that loop only trigger on the final frame (!iter.hasMoreFrames()), and FunCall fixup only happens when we are not building the final frame.

Depends on D89515

Comment 8

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 8: Add BaselineStackBuilder::buildExpressionStack r=jandem

The invalidate argument to BailoutIonToBaseline and InitFromBailout was only used for a single assertion in the IonReturnOverride case, which could be more directly asserted in jit::Bailout.

Depends on D89516

Comment 9

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 9: Add BaselineStackBuilder::validateFrame r=jandem

Depends on D89517

Comment 10

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 10: Add BaselineStackBuilder::finishLastFrame r=jandem

Depends on D89518

Comment 11

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 11: Move script, function, and exception info logic inside BaselineStackBuilder r=jandem

In preparation for the next patch, get rid of the out-parameters in InitFromBailout and clean a few things up.

Depends on D89519

Comment 12

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 12: Add BaselineStackBuilder::prepareForNextFrame r=jandem

This patch hoists the stub frame / rectifier frame code into its own function. The next patch splits that function up.

Depends on D89520

Comment 13

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 13: Split up BaselineStackBuilder::prepareForNextFrame r=jandem

Depends on D89521

Comment 14

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 14: Change InitFromBailout to BaselineStackBuilder::buildOneFrame r=jandem

Tying everything together.

Depends on D89522

Comment 15

[Iain Ireland [:iain]]

Attachment: Bug 1662915: Part 15: Update comments r=jandem

I was thinking about writing an [SMDOC] comment until I realized we already had one. I settled for a few updates to that comment and some annotations on the ASCII diagrams.

Depends on D89523

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/404660b9b7f6
Part 1: Change BaselineStackBuilder from struct to class r=jandem
https://hg.mozilla.org/integration/autoland/rev/79ce9d01a6a4
Part 2: Add BaselineStackBuilder::initFrame r=jandem
https://hg.mozilla.org/integration/autoland/rev/423aef1b78af
Part 3: Add BaselineStackBuilder::buildBaselineFrame r=jandem
https://hg.mozilla.org/integration/autoland/rev/b1459d3497d9
Part 4: Add BaselineStackBuilder::nextFrame r=jandem
https://hg.mozilla.org/integration/autoland/rev/6159158715b1
Part 5: Add BaselineStackBuilder::buildArguments r=jandem
https://hg.mozilla.org/integration/autoland/rev/d227307717a9
Part 6: Add BaselineStackBuilder::buildFixedSlots r=jandem
https://hg.mozilla.org/integration/autoland/rev/469ebee4d1d8
Part 7: Add BaselineStackBuilder::fixUpCallerArgs r=jandem
https://hg.mozilla.org/integration/autoland/rev/3639b51e4d3d
Part 8: Add BaselineStackBuilder::buildExpressionStack r=jandem
https://hg.mozilla.org/integration/autoland/rev/da9017d4d6c4
Part 9: Add BaselineStackBuilder::validateFrame r=jandem
https://hg.mozilla.org/integration/autoland/rev/f9e7c5a6553d
Part 10: Add BaselineStackBuilder::finishLastFrame r=jandem
https://hg.mozilla.org/integration/autoland/rev/441a0c7d2ba0
Part 11: Move script, function, and exception info logic inside BaselineStackBuilder r=jandem
https://hg.mozilla.org/integration/autoland/rev/3e03de3f89d8
Part 12: Add BaselineStackBuilder::prepareForNextFrame r=jandem
https://hg.mozilla.org/integration/autoland/rev/7e5cced63786
Part 13: Split up BaselineStackBuilder::prepareForNextFrame r=jandem
https://hg.mozilla.org/integration/autoland/rev/b5c068fd6837
Part 14: Change InitFromBailout to BaselineStackBuilder::buildOneFrame r=jandem
https://hg.mozilla.org/integration/autoland/rev/cc1abe9d41dd
Part 15: Update comments r=jandem

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/404660b9b7f6
https://hg.mozilla.org/mozilla-central/rev/79ce9d01a6a4
https://hg.mozilla.org/mozilla-central/rev/423aef1b78af
https://hg.mozilla.org/mozilla-central/rev/b1459d3497d9
https://hg.mozilla.org/mozilla-central/rev/6159158715b1
https://hg.mozilla.org/mozilla-central/rev/d227307717a9
https://hg.mozilla.org/mozilla-central/rev/469ebee4d1d8
https://hg.mozilla.org/mozilla-central/rev/3639b51e4d3d
https://hg.mozilla.org/mozilla-central/rev/da9017d4d6c4
https://hg.mozilla.org/mozilla-central/rev/f9e7c5a6553d
https://hg.mozilla.org/mozilla-central/rev/441a0c7d2ba0
https://hg.mozilla.org/mozilla-central/rev/3e03de3f89d8
https://hg.mozilla.org/mozilla-central/rev/7e5cced63786
https://hg.mozilla.org/mozilla-central/rev/b5c068fd6837
https://hg.mozilla.org/mozilla-central/rev/cc1abe9d41dd