1143704 - Some bytecode emitter cleanup

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Attachment: Patch

Functions like Emit1 are pretty annoying to use because they return a ptrdiff_t, so all callers have to check if the result < 0. It'd be nicer if these functions returned a bool (or were infallible, but that can wait for now). I did that and then I ended up moving most functions into the BytecodeEmitter class, so that we don't need to pass cx and bce around everywhere.

Anyway, this (folded!) patch is not complete but requesting feedback from people to see if they think this is the right direction.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 8578067 [details] [diff] [review]
Patch

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

The general idea makes sense, though I only just scanned it lightly.  A few thoughts:
 - I wonder if most BCE fields could be private now.
 - With a bit of refactoring of BytecodeCompiler (perhaps just moving some functions into BytecodeEmitter.cpp), could the definition of BytecodeEmitter be moved into BytecodeEmitter.cpp so the whole thing was just an impl detail of emitting?

Comment 2

[Luke Wagner [:luke]]

On second thought (re: my second bullet), BCE is not any worse, and symmetric to, Parser.  This is just an instance of the problem that C++'s classes force you to put a lot of private details in the public interface description of a class.  We could use ye olde PIMPL idiom to work around this, but that seems like overkill.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8578067 [details] [diff] [review]
Patch

Yeah, this has been on my cleanup todo list forever.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Make Emit1/Emit2 return bool

Big but mechanical.

EmitLoopEntry was the only place where we actually wanted the offset. That was easily fixed by calling bce->offset() before the Emit1 call, as we do elsewhere.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Make Emit3/EmitCall/EmitBackPatchOp return bool

Also straight-forward. The only wrinkle is that EmitGoto uses the offset returned by EmitBackPatchOp, but we can use the *lastp outparam there. All other callers just need a bool.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Make EmitLoopHead return a bool

EmitLoopHead returns a ptrdiff_t, but it's clearer to have it return a bool and let the callers do |top = bce->offset();| before the call.

We were already doing that for all for-loops, with this patch we do the same for do and while loops.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Move Emit1 into BytecodeEmitter

Large but mechanical change.

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Add a 'cx' to BytecodeEmitter

We'll no longer pass a cx to every function, and using sc->context requires an extra dereference, so this patch just adds a cx to BytecodeEmitter.

(I used 'cx' and not 'context' so that I don't have to rename it when converting functions to BytecodeEmitter methods.)

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Move more functions into BytecodeEmitter

More of the same. Note that all these methods have to be public for now; once we're done we can make most of them private.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 7 - More of the same

This patch also replaces the EMIT_UINT16_IMM_OP macro with BytecodeEmitter::emitUint16Operand.

BackPatch always returns true so I made BytecodeEmitter::backPatch return void.

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Move more methods

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 9 - Even more methods

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 10 - And even more methods

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 11 - Even more

Because BytecodeEmitter::backPatch is now infallible, BytecodeEmitter::popStatement (was PopStatementBCE) always returns true and can just return void.

Other than that it's just more mechanical changes.

Comment 15

[Jan de Mooij [:jandem]]

There will be a few more parts, but those can wait for now. After that I hope we can make most BytecodeEmitter fields/methods private.

It might also be nice to make emit1 and friends infallible and set an OOM flag that we can check at the end (just like the JIT's assemblers etc) but that's more complicated and not for this bug.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8578507 [details] [diff] [review]
Part 1 - Make Emit1/Emit2 return bool

Good one.

Comment 17

[Luke Wagner [:luke]]

Comment on attachment 8578560 [details] [diff] [review]
Part 8 - Move more methods

I see at this point in the patch queue there are still some more ptrdiff_t-returning emit calls.  Do you intend to convert them all to bool?  Otherwise I'm a bit scared that some inevitable <0 on bool or ! on ptrdiff_t mistakes will arise.

Comment 18

[Jan de Mooij [:jandem]]

(In reply to Luke Wagner [:luke] from comment #17)
> I see at this point in the patch queue there are still some more
> ptrdiff_t-returning emit calls.  Do you intend to convert them all to bool? 
> Otherwise I'm a bit scared that some inevitable <0 on bool or ! on ptrdiff_t
> mistakes will arise.

Right, this is also a problem without my patches because some Emit* functions already return bool.

Most callers of emitJump, emitGoto etc do need the offset, so I didn't change them. I should probably make them return bool and add a ptrdiff_t outparam, so that we can eliminate the <0 or >=0 tests completely.

Comment 19

[Brian Hackett [Laid off!]]

It would be nice if the bytecode emitter methods were all infallible, or at least infallible to the greatest degree possible.  If an allocation fails, just do nothing and set a flag on the emitter which causes things to fail later on, in the same fashion as the JIT macro assembler and other passes.  If we need to backpatch emitted bytecode and have a possibly bogus offset, check the flag before writing.  This is a little more complicated than that case because the emitter can fail for reasons other than OOM, but it seems like the emitter could just remember the first error as an exception object or frontend::CompileError and then throw it when it finally does get around to failing.

Comment 20

[Jan de Mooij [:jandem]]

(In reply to Brian Hackett (:bhackett) from comment #19)
> It would be nice if the bytecode emitter methods were all infallible, or at
> least infallible to the greatest degree possible.

Agreed, see also comment 15. Maybe I'll do this as a follow-up..

> This is a little more complicated than that
> case because the emitter can fail for reasons other than OOM, but it seems
> like the emitter could just remember the first error as an exception object
> or frontend::CompileError and then throw it when it finally does get around
> to failing.

I think a good first step is to make only the "trival" methods like emit1 infallible, because that's where the error-checking is most annoying (because there are so many calls to them..)

Comment 21

[Jeff Walden [:Waldo]]

Comment on attachment 8578535 [details] [diff] [review]
Part 5 - Add a 'cx' to BytecodeEmitter

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

Feh, contexts.

Comment 22

[Shu-yu Guo [:shu]]

Comment on attachment 8578568 [details] [diff] [review]
Part 9 - Even more methods

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

Seems reasonable and mechanical. FYI I didn't pore over every line.

Comment 23

[Eric Faust [:efaust]]

Comment on attachment 8578550 [details] [diff] [review]
Part 6 - Move more functions into BytecodeEmitter

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

I like how incrementally this can be done. Nice work. r=me

Comment 24

[Eric Faust [:efaust]]

Comment on attachment 8578572 [details] [diff] [review]
Part 10 - And even more methods

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

Well done.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +4899,4 @@
>          return false;
>  
>      if (type == STMT_SPREAD)
> +        stackDepth++;

Thoughts on making this, and lower references into |this->stackDepth|? I personally find it annoying to see bare names used this way. It costs me time looking for the local definition. It looks like you already did this for the code involving |loopDepth|, so let's just make it universal.

Comment 25

[Jan de Mooij [:jandem]]

(In reply to Eric Faust [:efaust] from comment #24)
> Thoughts on making this, and lower references into |this->stackDepth|? I
> personally find it annoying to see bare names used this way. It costs me
> time looking for the local definition. It looks like you already did this
> for the code involving |loopDepth|, so let's just make it universal.

Good point. I think when I make these fields private, I'll rename them to stackDepth_/loopDepth_, to make it clear these are class members.

Comment 26

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c86e48ac7dd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/9afadd15061c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b865279726
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf65c34bb34f
https://hg.mozilla.org/integration/mozilla-inbound/rev/19f500bbb3dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/443b2f31fd04
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f12cc5b2d67
https://hg.mozilla.org/integration/mozilla-inbound/rev/b30ab174b8a9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f85966886061
https://hg.mozilla.org/integration/mozilla-inbound/rev/af3aa3ee5b41
https://hg.mozilla.org/integration/mozilla-inbound/rev/787394ba34a2

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c86e48ac7dd3
https://hg.mozilla.org/mozilla-central/rev/9afadd15061c
https://hg.mozilla.org/mozilla-central/rev/a5b865279726
https://hg.mozilla.org/mozilla-central/rev/bf65c34bb34f
https://hg.mozilla.org/mozilla-central/rev/19f500bbb3dc
https://hg.mozilla.org/mozilla-central/rev/443b2f31fd04
https://hg.mozilla.org/mozilla-central/rev/4f12cc5b2d67
https://hg.mozilla.org/mozilla-central/rev/b30ab174b8a9
https://hg.mozilla.org/mozilla-central/rev/f85966886061
https://hg.mozilla.org/mozilla-central/rev/af3aa3ee5b41
https://hg.mozilla.org/mozilla-central/rev/787394ba34a2

Comment 28

[Jan de Mooij [:jandem]]

Attachment: Part 12 - Remaining methods

Comment 29

[Jan de Mooij [:jandem]]

Attachment: Part 13 - Return bool instead of ptrdiff_t

This changes emitJump/emitCheck/emitN to return bool instead of intptr_t, with an intptr_t* outparam where necessary.

I considered changing the type to size_t/unsigned, since the offset is always positive now, but we subtract these values to determine jump offsets etc, so I kept intptr_t to avoid subtle sign issues.

Comment 30

[Jan de Mooij [:jandem]]

Attachment: Part 14 - Make newSrcNote* return bool

Make newSrcNote* return bool with an optional unsigned* index outparam, instead of an int return type.

With this patch there are no more "< 0" or ">= 0" OOM checks in BytecodeEmitter.cpp

Comment 31

[Luke Wagner [:luke]]

Comment on attachment 8582337 [details] [diff] [review]
Part 14 - Make newSrcNote* return bool

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

Nice

Comment 32

[Jan de Mooij [:jandem]]

Parts 12-14:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0c37c1837b
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2113e37300
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6044f33b3e4

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ff0c37c1837b
https://hg.mozilla.org/mozilla-central/rev/5e2113e37300
https://hg.mozilla.org/mozilla-central/rev/b6044f33b3e4

Comment 34

[Jan de Mooij [:jandem]]

Closing this as all patches landed. There's always more we can do but it's clearer to file new bugs.