1284719 - Remove the bytecode main/prologue split

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Shu-yu Guo [:shu]]

After the rewrite, the prologue can be removed. Things will be emitted in the right order up front, no need to switch to prologue due to function hoisting.

Comment 1

[Shu-yu Guo [:shu]]

One problem is that the Debugger considers ops in the prologue to be unobservable, which affects the ability to set a breakpoint on the op. If we were to get rid of the prologue, we need a more robust notion of which ops can be breakpoint targets.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Shu-yu Guo [:shu] from comment #1)
> One problem is that the Debugger considers ops in the prologue to be
> unobservable, which affects the ability to set a breakpoint on the op. If we
> were to get rid of the prologue, we need a more robust notion of which ops
> can be breakpoint targets.

Maybe a JSOP_START or JSOP_BEGIN op, where we currently switch to main?

Comment 3

[Nicolas B. Pierron [:nbp]]

I thought we only allowed breakpoints on bytecode which have new-line information?  Does the prologue have any line notes?

Comment 4

[Shu-yu Guo [:shu]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> I thought we only allowed breakpoints on bytecode which have new-line
> information?  Does the prologue have any line notes?

The FlowGraphSummary used by Debugger explicitly uses offsets relative to script->main(), not script->code() right now.

Comment 5

[Jan de Mooij [:jandem]]

Hah, I was just filing a bug for this. I prototyped some patches for this that are green so far, the main issue is DEFFUN ops in global/eval scripts need to be emitted upfront. My patches store these functions in a Vector in {Global,Eval}SharedContext.

https://hg.mozilla.org/try/rev/5f5e70e89ebcec97b77c7055bc20659321d3cd4e
https://hg.mozilla.org/try/rev/b35443b316ecee7535b9cec7e3ccc462df16a23b

Emitting everything linearly will simplify some IC work I want to do later.

arai, what do you think?

Comment 6

[Tooru Fujisawa [:arai]]

Looks great :)

I don't see any issue so far,
but I want to make sure how it affects source notes for prologue bytecodes, will look into the details shortly.
(it's related to bug 1519005 patch)

Comment 7

[Tooru Fujisawa [:arai]]

so, in the patch, hoisted/top-level functions' bytecode are emitted in the different place than where the source notes are emitted (emitHoistedFunctionsInList)
now I think we really don't need the source notes for them.

also, hoisted/top-level function's JSFunction/FunctionBox are still modified inside BytecodeEmitter::emitFunction.

• https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/js/src/frontend/BytecodeEmitter.cpp#5510
• https://searchfox.org/mozilla-central/rev/dac799c9f4e9f5f05c1071cba94f2522aa31f7eb/js/src/frontend/BytecodeEmitter.cpp#5516

I wonder if we can factor out that part into the different function and use it both from both BytecodeEmitter::{emitFunction,defineTopLevelFunctions}, and completely skip hoisted/top-level functions in emitFunction.
but maybe it's for other bug (part of bug 1473796 maybe?).

anyway, the patch looks good to me :D

Comment 8

[Jan de Mooij [:jandem]]

Thanks arai.

I didn't see emitHoistedFunctionsInList yesterday, I think we can use that for this too. I'll try this today.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1284719 part 1 - Emit hoisted top-level functions directly in the prologue, remove switchToPrologue. r?arai!

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1284719 part 2 - Fold EmitSection into BytecodeEmitter, remove prologue/main split. r?arai!

Depends on D16947

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d260750a87e
part 1 - Emit hoisted top-level functions directly in the prologue, remove switchToPrologue. r=arai
https://hg.mozilla.org/integration/autoland/rev/83d6478d2611
part 2 - Fold EmitSection into BytecodeEmitter, remove prologue/main split. r=arai

Comment 12

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/1d260750a87e
https://hg.mozilla.org/mozilla-central/rev/83d6478d2611