1261826 - Add JSOP_JUMPTARGET when emitting bytecode.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

Currently, when --ion-pgo=on is used, the interpreter is switching to a mode where we profile the generated code.  At the moment, this only relies on the EnableInterrupts fake JSOP, which is made to support any debugging information.

This is inefficient, and slow down the Interpreter by a factor larger than 2 on Sunspider (when the jits are disabled).

The interpreter still matters a bit for code which is executed only once.

Thus I suggest to remove the need for using the EnableInterrupts code path, by adding an JSOP_PCCOUNT at each jump/throw targets.  This new opcode should have no associated information and the pc will still be the index of the counters.

One thing that we might want to investigate later, would be if such instruction would not be better used as marking the top of basic blocks, thus removing the need for having some reverse engineering based on source notes in Baseline / Ion.

Comment 1

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - JSScript::fullyInitFromEmitter explicitly use 'main' instead of 'current'.

Comment 2

[Nicolas B. Pierron [:nbp]]

Attachment: part 1.5 - Add JSScript::assertValidJumpTargets.

This code is inspired from JSScript::initScriptCounts except that append
operations are replaced by MOZ_ASSERT.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8737822 [details] [diff] [review]
part 1 - JSScript::fullyInitFromEmitter explicitly use 'main' instead of 'current'.

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

Yikes.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 8737885 [details] [diff] [review]
part 1.5 - Add JSScript::assertValidJumpTargets.

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

Very nice.

It's weird how we have tons and tons of code in the engine to parse and analyze bytecode (in the jits, the debugger, the decompiler, etc.) and yet we're never able to reuse any of it. I wonder if that means we're doing something wrong.

::: js/src/jsscript.cpp
@@ +3124,5 @@
> +JSScript::assertValidJumpTargets() const
> +{
> +    jsbytecode* end = codeEnd();
> +    jsbytecode* mainEntry = main();
> +    for (jsbytecode* pc = code(); pc != end; pc = GetNextPc(pc)) {

There's a BytecodeRange class in jsopcodeinlines.h for iterating over bytecodes. Maybe it's not worth using here, though.

@@ +3127,5 @@
> +    jsbytecode* mainEntry = main();
> +    for (jsbytecode* pc = code(); pc != end; pc = GetNextPc(pc)) {
> +        // Check jump instructions' target.
> +        bool jump = IsJumpOpcode(JSOp(*pc));
> +        if (jump) {

Style nit: `if (IsJumpOpcode(...))` please

@@ +3131,5 @@
> +        if (jump) {
> +            jsbytecode* target = pc + GET_JUMP_OFFSET(pc);
> +            MOZ_ASSERT(mainEntry <= target && target < end);
> +
> +            // Check falltrhough of conditional jump instructions.

typo: "fallthrough"

Comment 5

[Nicolas B. Pierron [:nbp]]

Attachment: BytecodeEmitter: Distinguish offsets based on their purposes.

This patch add a new structure named Offset which is using a phantom type to
distinguish what is the purpose of the ptrdiff_t that it contains.

 - BranchTarget:  This is the offset of the instruction which would be the
   target of a branch.  For the moment, it could be written as

     Offset<BranchTarget> target{ offset() }

   But for the purpose of this bug, I added an extra fallible emit function,
   as we might want to create a JSOP_PCCOUNT later for most of the branch
   target.

 - BranchSingle:  This is the offset of a single jump, which would be fixed
   by setJumpOffset.

 - BranchChain:  This is the offset of the last jump instruction, which has
   to be patched with the backPatch function.

A part from adding semantic to the current ptrdiff_t, this also do the
following modification:

 - Change the emitLogical function to use the backPatch function.
 - Invert the backPatch function delta (jump by adding the offset)
 - Type system based self-documentation.

What do you think?

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> Created attachment 8739162 [details] [diff] [review]
> BytecodeEmitter: Distinguish offsets based on their purposes.

 - One other advantage, is that we are now manipulating offsets instead of jsbytecode pointers.  Thus there is less risk of introducing issues with relocated vectors in the future.

 - I removed the extra "0" argument given to a lot of "emitJump" function calls.

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8739162 [details] [diff] [review]
BytecodeEmitter: Distinguish offsets based on their purposes.

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

I think this doesn't go far enough. Reduce complexity. Delete some methods.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +90,2 @@
>          MOZ_ASSERT(type == StmtType::TRY || type == StmtType::FINALLY);
> +        return reinterpret_cast<Offset<BranchSingle>&>(continues);

These reinterpret_casts have to go. I think this is technically undefined behavior, but in any case it is worth avoiding.

::: js/src/frontend/BytecodeEmitter.h
@@ +128,5 @@
>      // Annex B block-scoped function semantics.
>      AnnexB,
>  };
>  
> +// Annotate the offsets with a phantom type, which serves as a way to help the

OK, I know what a phantom type is, but it doesn't strike me as the right tool for this.

Instead, maybe it should be like this:

    BytecodeEmitter::emitJump(JSOp op, Offset target);
    BytecodeEmitter::emitJumpToPatchLater(JSOp op, JumpList* jumps);
    BytecodeEmitter::patchJumps(JumpList* jumps, Offset target);

with either `typedef size_t Offset;` or a simple wrapper.

I think you mentioned that a single type can do the work of both BranchChain and BranchSingle, and it seems not worth having extra stuff just to avoid the loop, in the case where we know there will be exactly one jump to back-patch. I agree. Simplify away.

Comment 8

[Nicolas B. Pierron [:nbp]]

(In reply to Jason Orendorff [:jorendorff] from comment #7)
> with either `typedef size_t Offset;` or a simple wrapper.

I used a simple wrapper as opposed to a typedef/template-using to produce error messages at compile time.
Which also guarantee the completeness of the patch.

> I think you mentioned that a single type can do the work of both BranchChain
> and BranchSingle, and it seems not worth having extra stuff just to avoid
> the loop, in the case where we know there will be exactly one jump to
> back-patch. I agree. Simplify away.

Ok, I will merge both, which will remove the reinterpret_cast from above.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: BytecodeEmitter: Distinguish offsets based on their purposes.

I am asking for feedback instead of review, because I think they are too
many calls to emitJumpTarget, where we do not necessary need to have a
dedicated instruction, either because there is already a dedicated
instruction, or because this is not a jump target.

In the mean time, this patch highlight the big lines which should remain
unless the feedback suggest otherwise:
 - Add JumpList and JumpTarget wrapper.
 - Add emitJump, emitJumpTarget and setJumpTarget.
 - Add a few wrppaer functions based on the previous one, to simplify common
   patterns, such as emitBackwardJump and emitJumpHere.

This patch removes the logic for handling single patchable jumps, and use
the back-patch logic for all the jumps.  Only accept JSOp on the emitJump
function, and no longer as part of the setJumpTarget.

Factor the setJumpTarget under emitLoopEntry, as this was a common pattern.

Note, I think it would be nicer if setJumpTarget would take a JumpList&
argument in order to neuter the offset to -2 in debug builds.  Otherwise the
assertions in setJumpTarget is able to catch reused JumpList.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 8741067 [details] [diff] [review]
BytecodeEmitter: Distinguish offsets based on their purposes.

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

Yeah, this is much nicer.

This looks really close to being done. Any performance numbers yet?

Bikeshedding: Can we call the new opcode JSOP_LABEL or JSOP_JUMPTARGET instead of JSOP_PCCOUNT?

Or maybe JSOP_COMEFROM: https://en.wikipedia.org/wiki/COMEFROM (joke)

Comment 11

[Nicolas B. Pierron [:nbp]]

(In reply to Jason Orendorff [:jorendorff] from comment #10)
> This looks really close to being done. Any performance numbers yet?

Not yet, I am still trying to get it correct for most of the locations out-side the bytecode emitter.

> Bikeshedding: Can we call the new opcode JSOP_LABEL or JSOP_JUMPTARGET
> instead of JSOP_PCCOUNT?

JSOP_LABEL is already used, I think I will go for JSOP_JUMPTARGET.  But for convenience, I hesitate to add a new JOF_TARGET, as to avoid adding too many jump target I also accept JSOP_LOOPHEAD / JSOP_LOOPENTRY as valid jump targets.

> Or maybe JSOP_COMEFROM: https://en.wikipedia.org/wiki/COMEFROM (joke)

If we were to do that, this would simplify a lot our decompiler iff we could also walk our bytecode in reverse.  So, I think we should leave that for a follow-up.

Comment 12

[Nicolas B. Pierron [:nbp]]

Ok, with the upcoming patches, I got the following results while running SunSpider benchmark with only the Interpreter.

Sunspider Interpreter        before                    after
    pgo disable         2562.8ms +/- 1.1%         2543.4ms +/- 1.3%
    pgo enabled         6321.4ms +/- 1.3%         3013.7ms +/- 0.4%

Comment 13

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Extract assertions from JSScript::fullyInitFromEmitter.

Comment 14

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Remove outdated comment about XDR_BYTECODE_VERSION.

Comment 15

[Tooru Fujisawa [:arai]]

Comment on attachment 8745330 [details] [diff] [review]
part 3 - Remove outdated comment about XDR_BYTECODE_VERSION.

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

Thank you :D

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - BytecodeEmitter: Distinguish offsets based on their purposes.

This patch adds 2 new types (JumpList and JumpTarget) which are wrapping a
ptrdiff_t field which represent an offset in the bytecode. These types are
used to distinguish the offsets types in the bytecode emitter.

Before this patch we had 2 ways of binding jump instruction to their target,
now we have a single way, which is the "setJumpTarget" function.  This
implies that the JumpList got created by using the "emitJump" function, and
that the target got created by using the "emitJumpTarget" function.

In a few cases:
 - The JumpTarget is set as being -1, when we do not expect any jump using
   this target, such as before "pushLoopStatement", where the "stmt.setTop"
   function is called later.

 - The JumpTarget is initialized to the current offset, when the next
   instruction being produced is assimilated as a jump target, as noted by
   the "BytecodeJumpTarget" function added in part 7 (with the proper
   assertions in "setJumpTarget" and "assertValidJumpTargets") 

As we do not need to change the opcode when we set the jump target, we no
longer need to have a JSOP_BACKPATCH, as all the jump instructions are
now back-patched.

Comment 17

[Nicolas B. Pierron [:nbp]]

Attachment: part 5 - Add a simple way to walk the bytecode until we find a consumer.

This fix an error which appears when part 7 is landed.
The problem being that we have a JSOP_JUMPTARGET inserted in the middle,
thus miss-leading the Detecting function of GetNonexistentProperty.  This
caused us to report errors about undefined references while testing for
undefined values.

  if (a.b)
      ...

This patch adds a FindConsumerPc function which looks for the consumer of
one of the definitions pushed on the stack.  This FindConsumerPc assumes
that the bytecode can safely be traversed lineraly.

Comment 18

[Nicolas B. Pierron [:nbp]]

Attachment: part 6 - Factor code which record new Bytecode offset stack.

This code factors out the code which records the offset-stack for the next
instructions.

This code is quite similar to what is done in the FlowGraphSummary::populate function.

Comment 19

[Nicolas B. Pierron [:nbp]]

Attachment: part 7 - Add a no-op bytecode to filter out branches results from the decompiler.

In part 8, we are adding the BytecodeJumpTarget in the
BytecodeParser::parse() function, to let it reuse the result from the
"addJump" calls.

Unfortunately, while this makes the BytecodeParser::parse function logically
correct, it caused us to have more offuscated error messages in
deconstructing assignments, as the following conditional

  (result.done ? undefined : result.value)

is correctly being interpreted as an "(intermediate value)".

This no-op is used to let the BytecodeParser ignore the content of the
"undefined" branch. Thus generating the same error message as we did before
with deconstructing patterns.

Comment 20

[Nicolas B. Pierron [:nbp]]

Attachment: part 8 - Add JSOP_JUMPTARGET opcode.

This add the JSOP_JUMPTARGET opcode on top of part 4 work.

This modified all the locations in the BytecodeParser (expression
decompiler), Debugger, Interpreter, Baseline, Ion.

In addition, this patch adds the BytecodeJumpTarget function, used to assert
that the BytecodeEmitter does correctly emitJumpTarget for the Jump
instructions, and uses it in the bytecode parser / the debugger to find
out when we should reload the content of the offset-stack / location.

Comment 21

[Nicolas B. Pierron [:nbp]]

Attachment: part 9 - initScriptCounts relies on jump targets for allocating PCCounts.

Use the newly added JSOP_JUMPTARGET from part 8, to allocate the PCCount
structure for each of them, instead of looking at jump computed targets.

Comment 22

[Nicolas B. Pierron [:nbp]]

Attachment: part 10 - Only increment code coverage counters on jump targets.

This patch only increment the PCCount counters only for jump target
instructions, as allocated by the initScriptCounts function, and as
indicated by the bytecode.

By no longer using the debug-mode of the interpreter, this last patch divide
by 2 the time spent in the interpreter when the pgo is enabled. (see comment 12)

Comment 23

[Nicolas B. Pierron [:nbp]]

Attachment: part 8.1 - Make Debugger.Script.getOffsetLocation only consider entry point locations.

Only location indicated by the isEntryPoint flag of
theBytecodeRangeWithPosition, or cached by the flowData are corresponding to
valid locations.

As other loops are filtering for isEntryPoint, I modified
Debugger.Script.getOffsetLocation to settle either on the next valid
flowData (the last isEntryPoint flowing at the current offset), or on the
next valid isEntryPoint.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 8745354 [details] [diff] [review]
part 8 - Add JSOP_JUMPTARGET opcode.

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

I only reviewed the JIT changes (but glanced at the other files).

::: js/src/jit/IonBuilder.cpp
@@ +2978,5 @@
>      // Find the target loop.
>      CFGState* found = nullptr;
>      jsbytecode* target = pc + GetJumpOffset(pc);
>      for (size_t i = loops_.length() - 1; i < loops_.length(); i--) {
> +        if (loops_[i].continuepc == target + 1 ||

Add a comment here, like // +1 to skip JSOP_JUMPTARGET.

::: js/src/jsopcode.h
@@ +431,5 @@
>      }
>  }
>  
> +static inline bool
> +BytecodeJumpTarget(JSOp op)

Nit: I'd call this BytecodeIsJumpTarget

::: js/src/vm/CodeCoverage.cpp
@@ +317,5 @@
>  
>              if (sc) {
> +                // Look for the last case entry before the default pc.
> +                lastcasepc = firstcasepc - 1;
> +                for (int j = 0; j < numCases; j++) {

size_t?

::: js/src/vm/Opcodes.h
@@ +2182,5 @@
>       */ \
> +    macro(JSOP_NOP_DESTRUCTURING, 229, "nop-destructuring", NULL, 1, 0, 0, JOF_BYTE) \
> +    /*
> +     * This opcode is a no-op and it indicates the location of a jump
> +     * instruction target.

Maybe mention that some instructions, like JSOP_LOOPHEAD etc, are implicit jump targets (we don't emit JSOP_JUMPTARGET).

Is that optimization worth it or necessary? JSOP_JUMPTARGET is a single byte and if we could just always use that it might simplify the code a bit and make it easier to build new things on top of it.

Comment 25

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #24)
> Comment on attachment 8745354 [details] [diff] [review]
> part 8 - Add JSOP_JUMPTARGET opcode.
> 
> Review of attachment 8745354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I only reviewed the JIT changes (but glanced at the other files).

Thanks.

> ::: js/src/vm/Opcodes.h
> @@ +2182,5 @@
> >       */ \
> > +    macro(JSOP_NOP_DESTRUCTURING, 229, "nop-destructuring", NULL, 1, 0, 0, JOF_BYTE) \
> > +    /*
> > +     * This opcode is a no-op and it indicates the location of a jump
> > +     * instruction target.
> 
> Maybe mention that some instructions, like JSOP_LOOPHEAD etc, are implicit
> jump targets (we don't emit JSOP_JUMPTARGET).
> 
> Is that optimization worth it or necessary? JSOP_JUMPTARGET is a single byte
> and if we could just always use that it might simplify the code a bit and
> make it easier to build new things on top of it.

This was my first attempt, but the amount of changes in IonBuilder would have been harder to managed.  The problem is that IonBuilder make so much assumption on the bytecode layout, that adding a JUMPTARGET above it would be even more confusing in IonBuilder.

Comment 26

[Shu-yu Guo [:shu]]

Comment on attachment 8745339 [details] [diff] [review]
part 4 - BytecodeEmitter: Distinguish offsets based on their purposes.

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

Thanks for doing this cleanup. Once I got past the names this made pretty good sense, but the names really did get in the way of understanding what was going on.

My suggestions:

emitJumpTarget -> setJumpTarget
setJumpTarget -> patchJumpsToTarget
emitJumpHere -> patchJumpsToHere

::: js/src/frontend/BytecodeEmitter.cpp
@@ +315,5 @@
>  bool
> +BytecodeEmitter::emitJumpTarget(JumpTarget* target)
> +{
> +    target->offset = offset();
> +    return true;

I'm confused. The method is named emitJumpTarget but nothing is emitted, and it is infallible but returns bool. Perhaps should be renamed in conjunction with the equally confusingly named 'setJumpTarget' below.

@@ +343,5 @@
> +    if (BytecodeFallsThrough(op)) {
> +        JumpTarget fallthrough;
> +        if (!emitJumpTarget(&fallthrough))
> +            return false;
> +    }

What does this if do? |fallthrough| is never used and |emitJumpTarget| just assigns the current offset to it.

@@ +375,5 @@
> +        MOZ_ASSERT(delta < 0);
> +        ptrdiff_t span = targetPc - pc;
> +        SET_JUMP_OFFSET(pc, span);
> +        pc += delta;
> +    }

This function needs a comment or a rename. Other methods that are named 'setX' regarding jumps are doing simple mutation of a field. This is computing the transitive closure of jump targets to remove intermediate jumps, correct?

@@ +379,5 @@
> +    }
> +}
> +
> +bool
> +BytecodeEmitter::emitJumpHere(JumpList jump)

Is this more like "make the list of jumps that start with |jump| jump to the current location"? Maybe 'jumpToHere'? As it is it could be read as "emit a jump at this location, i.e., from here".

@@ +560,5 @@
> +{
> +    /* Set loop and enclosing "update" offsets, for continue. */
> +    do {
> +        stmt->update = target;
> +    } while ((stmt = stmt->enclosing) != nullptr && stmt->type == StmtType::LABEL);

Existing style nit: not a fan of this (stmt = stmt->enclosing) != nullptr style. Could you lift that out of the condition?

::: js/src/frontend/BytecodeEmitter.h
@@ +130,5 @@
>  };
>  
> +// List of jump instructions. This structure stores the offset of the last
> +// emitted jump instruction, while the jump instruction offset stores the
> +// relative offset of the next jump instruction in the list.

It took me a while to understand why this named List and what this comment means.

I would have an example showing that the list is an in-bytecode list connected by jump offsets, and this points to the beginning of it.

Example usage of this in conjunction with the setJump/emitJump methods below would be extra nice.

Comment 27

[Shu-yu Guo [:shu]]

Comment on attachment 8745339 [details] [diff] [review]
part 4 - BytecodeEmitter: Distinguish offsets based on their purposes.

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

::: js/src/frontend/BytecodeEmitter.cpp
@@ +315,5 @@
>  bool
> +BytecodeEmitter::emitJumpTarget(JumpTarget* target)
> +{
> +    target->offset = offset();
> +    return true;

Oh this does actually emit something later in the patch series. I take this comment back then.

Comment 28

[Shu-yu Guo [:shu]]

Comment on attachment 8745354 [details] [diff] [review]
part 8 - Add JSOP_JUMPTARGET opcode.

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

r=me with the TRY case in FlowGraphSummary removed or explained.

::: js/src/vm/Debugger.cpp
@@ +5240,5 @@
> +            isEntryPoint = true;
> +        }
> +
> +        if (isEntryPoint && frontOpcode() == JSOP_JUMPTARGET) {
> +            wasArtifactEntryPoint = isEntryPoint;

Just change this to wasArtifactEntryPoint = true.

@@ +5458,5 @@
> +                        uint32_t catchOffset = startOffset + tn->length;
> +                        if (tn->kind == JSTRY_CATCH || tn->kind == JSTRY_FINALLY)
> +                            addEdge(lineno, column, catchOffset);
> +                    }
> +                }

This doesn't seem right. Exception edges aren't regular control flow edges, and a catch/finally block should not be considered reachable from the try itself.

Comment 29

[Shu-yu Guo [:shu]]

Comment on attachment 8745450 [details] [diff] [review]
part 8.1 - Make Debugger.Script.getOffsetLocation only consider entry point locations.

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

::: js/src/vm/Debugger.cpp
@@ +5673,5 @@
>  
> +    offset = r.frontOffset();
> +    bool isEntryPoint = r.frontIsEntryPoint();
> +
> +    // Line numbers are only correctly defined on entry points. Thus looks

Typo: thus -> this

Comment 30

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #26)
> My suggestions:
> 
> setJumpTarget -> patchJumpsToTarget

I am not completely convinced by this name, I will wait for Jason ideas.
Alternative I can think of are:

  {set,patch,bind,update}Jumps{Target,Destination,Label,Offset,}{To,}

"patch-to" sounds weird to my hears.

I am tempted by

  bindJumpsTo(JumpList, JumpTarget)


> @@ +343,5 @@
> > +    if (BytecodeFallsThrough(op)) {
> > +        JumpTarget fallthrough;
> > +        if (!emitJumpTarget(&fallthrough))
> > +            return false;
> > +    }
> 
> What does this if do? |fallthrough| is never used and |emitJumpTarget| just
> assigns the current offset to it.

Once emitJumpTarget is no-longer a no-op (part 8), this would be used to mark the beginning of a block. The fallthrough value is not used, but I preferred to go with calling emitJumpTarget to have a single function responsible for making jump targets, than repeating its content.

> @@ +375,5 @@
> > +        MOZ_ASSERT(delta < 0);
> > +        ptrdiff_t span = targetPc - pc;
> > +        SET_JUMP_OFFSET(pc, span);
> > +        pc += delta;
> > +    }
> 
> This function needs a comment or a rename. Other methods that are named
> 'setX' regarding jumps are doing simple mutation of a field. This is
> computing the transitive closure of jump targets to remove intermediate
> jumps, correct?

If the target were part of the list, I would have say yes.
Here is the comment that I would add to the JumpList structure:

// Example:
//
//     JumpList brList;
//     if (!emitJump(JSOP_IFEQ, &brList))
//         return false;
//     ...
//     JumpTarget label;
//     if (!emitJumpTarget(&label))
//         return false;
//     ...
//     if (!emitJump(JSOP_GOTO, &brList))
//         return false;
//     ...
//     setJumpTarget(brList, label);
//
//                 +-> -1
//                 |
//                 |
//    ifeq ..   <+ +                +-+   ifeq .. 
//    ..         |                  |     ..
//  label:       |                  +-> label:
//    jumptarget |                  |     jumptarget
//    ..         |                  |     ..
//    goto .. <+ +                  +-+   goto .. <+
//             |                                   |
//             |                                   |
//             +                                   +
//           brList                              brList
//
//       |                                 ^
//       +--------- setJumpTarget ---------+
//

Comment 31

[Brian Hackett [Laid off!]]

Comment on attachment 8745359 [details] [diff] [review]
part 10 - Only increment code coverage counters on jump targets.

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

This looks OK, but looking over this bug is leaving me pretty confused.

Why do we care what slowdown the interpreter has when profiling script execution?  I thought that sometime after baseline landed the interpreter basically became irrelevant to performance, hence the removal of interpreter specific optimizations like the per-runtime property cache.  Is this no longer true?

Besides the complexity these patches add, they also have a couple negative effects.  Most importantly, the JSOP_JUMPTARGET opcode is another opcode that shows up in bytecode streams, and will bloat the allocated memory used by the scripts.  What effect does this patch have on script bytecode sizes, on real workloads?  Less importantly, by making the interpreter more efficient when profiling, the interpreter will become less efficient when not profiling --- it has to process these JSOP_JUMPTARGET opcodes and repeatedly test whether the script has profiling enabled.

Comment 32

[Nicolas B. Pierron [:nbp]]

Note, these counters are use for the code coverage, but they are also used for branch pruning optimization.  So knowing we might want to enable branch pruning by default, the goal here is to make sure we do not keep obvious regressions.

(In reply to Brian Hackett (:bhackett) from comment #31)
> Why do we care what slowdown the interpreter has when profiling script
> execution?  I thought that sometime after baseline landed the interpreter
> basically became irrelevant to performance, hence the removal of interpreter
> specific optimizations like the per-runtime property cache.  Is this no
> longer true?

We care, because the current profiling scheme involve turning on the Debugger infrastructure while running the interpreter which slow it down by more than a factor a 2. (comment 12)

I think we still care about the performance of the interpreter, as it remains the first thing which is being executed.  So even if this is 10 iterations max of every new function, we don't want them to be twice as slow for these 10 iterations.

> Besides the complexity these patches add, they also have a couple negative
> effects.

I agree on the complexity of making these patches, on the other hand I disagree on the complexity and on the correctness of the out-come.

> Most importantly, the JSOP_JUMPTARGET opcode is another opcode
> that shows up in bytecode streams, and will bloat the allocated memory used
> by the scripts.  What effect does this patch have on script bytecode sizes,
> on real workloads?

I will check.

> Less importantly, by making the interpreter more
> efficient when profiling, the interpreter will become less efficient when
> not profiling --- it has to process these JSOP_JUMPTARGET opcodes and
> repeatedly test whether the script has profiling enabled.

Let me contradict you with what I benchmarked in comment 12.

Comment 33

[Jan de Mooij [:jandem]]

I agree with Nicolas that we don't want to make the interpreter significantly slower.

(In reply to Nicolas B. Pierron [:nbp] from comment #32)
> Note, these counters are use for the code coverage, but they are also used
> for branch pruning optimization.  So knowing we might want to enable branch
> pruning by default, the goal here is to make sure we do not keep obvious
> regressions.

For branch pruning, how feasible is it to start counting once we enter Baseline? Type profiling also starts working once we enter Baseline, and if a branch hits in the interpreter but then never in Baseline, Ion can assume it won't hit again.

Comment 34

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #33)
> For branch pruning, how feasible is it to start counting once we enter
> Baseline? Type profiling also starts working once we enter Baseline, and if
> a branch hits in the interpreter but then never in Baseline, Ion can assume
> it won't hit again.

I would have to try, to see the consequences on branch pruning optimization, but if this does not damage performances then this could save the 20% hit we have on the interpreter.

Still, I guess this is a slippery path as this is removing the first 10 iterations, and I saw issues related to the removal of branches which were infrequent ~1/200 hits.  Also, I guess that if we have no type info, then we are also likely to bailout anyway.

Comment 35

[Nicolas B. Pierron [:nbp]]

(In reply to Shu-yu Guo [:shu] from comment #28)
> Comment on attachment 8745354 [details] [diff] [review]
> part 8 - Add JSOP_JUMPTARGET opcode.
> 
> Review of attachment 8745354 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the TRY case in FlowGraphSummary removed or explained.
> 
> @@ +5458,5 @@
> > +                        uint32_t catchOffset = startOffset + tn->length;
> > +                        if (tn->kind == JSTRY_CATCH || tn->kind == JSTRY_FINALLY)
> > +                            addEdge(lineno, column, catchOffset);
> > +                    }
> > +                }
> 
> This doesn't seem right. Exception edges aren't regular control flow edges,
> and a catch/finally block should not be considered reachable from the try
> itself.

Ok, I investigated a bit more, since what you mention sounds logical.

When the code to handle JSOP_TRY is not present, the problem comes from from the newly added branch in FlowGraphSummary::populate function, which use the JSOP_JUMPTARGET as a hint for taking the location of the initial branch:

            if (BytecodeIsJumpTarget(op)) {
                lineno = entries_[r.frontOffset()].lineno();
                column = entries_[r.frontOffset()].column();
            }

The bytecode we generate in js::frontend::BytecodeEmitter::emitTry is the following:

  loc     op
  -----   --
  […]
  00022:  try
  […]
  00035:  throw
  00036:  gosub 91 (+55)
  00041:  jumptarget
  00042:  goto 107 (+65)
tryEnd:
  00047:  jumptarget
  00048:  undefined
  00049:  setrval
  ->  00050:  pushblockscope depth 0 {e: 0}
  […]
  00080:  gosub 91 (+11)
  00085:  jumptarget
  00086:  goto 107 (+21)
finallyStart:
  00091:  jumptarget
  00092:  finally
  00093:  getrval
  00094:  undefined
  00095:  setrval
  […]
  00104:  setrval
  00105:  retsub
  00106:  nop
  00107:  jumptarget

When we walk on the offset 47, we reset the lineno and column to the value initialized by default to "hasNoEdges".  This value is copied over and over until we hit the first instruction which a valid entry point, at offset 50.

Under DebuggerScript_getLineOffsets, we are looking for all entry points which have a valid incoming edge.  Thus excluded the offset 50 from the list, causing debug/Frame-onStep-11.js to fail.

Before, this code used to work because, even if we did not copy the line number after a non-fallthrough instructions, we did not reset the lineno & column values.  Which caused us to propagate the lineno & column to the next instruction after a non-fallthrough instruction, thus copying the location information to another part of the control flow.

The code to handle JSOP_TRY is made to ensure that we have incoming flow-graph edges for the catch statement.  Thus copying the lineno & column of the JSOP_TRY instead of propagating the lineno & column of the end of the try block.

This does not change the behaviour as the lineno & column of entry points, such as offset 50.  On the other hand, this change the lineno & column of the predecessor of the catch, making it be the "try {" statement instead of being the last line of the try-body.

I will add a similar comment into the code, without references to the previous behaviour.

Comment 36

[Nicolas B. Pierron [:nbp]]

Attachment: Script density by size, and cumulated size.

(In reply to Brian Hackett (:bhackett) from comment #31)
> Comment on attachment 8745359 [details] [diff] [review]
> part 10 - Only increment code coverage counters on jump targets.
> 
> Review of attachment 8745359 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Most importantly, the JSOP_JUMPTARGET opcode is another opcode
> that shows up in bytecode streams, and will bloat the allocated memory used
> by the scripts.  What effect does this patch have on script bytecode sizes,
> on real workloads?

I recorded 2 sessions of Firefox where I browse on google, youtube, facebook, and wikipedia.  I had to normalize the loaded script by script density in function of their size.

We we had a major increase we should be able to spot it by seeing the curve being shifted by the amount by which it is being increased, but the noise is making this hard to spot.  On the other hand the integration of (likelyhood(size) * size) over the size highlight that this is a script size increase of 5.8%.

I think I have a way to reduce this a bit at the cost of aliasing of the branch sources, by folding cases when multiple jump target are being emitted in a row.

Comment 37

[Nicolas B. Pierron [:nbp]]

(In reply to Jan de Mooij [:jandem] from comment #33)
> For branch pruning, how feasible is it to start counting once we enter
> Baseline?

I will look at that while I investigate Bug 1263645, as this sounds like a similar issue where the code coverage purpose is to debug with correct monotonous information, and with the optimization aspect where the counters are used as a hint for averaging the profile info.

Comment 38

[Jason Orendorff [:jorendorff]]

Comment on attachment 8745339 [details] [diff] [review]
part 4 - BytecodeEmitter: Distinguish offsets based on their purposes.

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

Sorry for the long delay here.

I agree with shu's comments that some methods should be renamed; in particular, methods that patch previously emitted bytecode should have "patch" in the name.

For example, I think
  emitJumpHere -> emitJumpTargetAndPatch

::: js/src/frontend/BytecodeEmitter.cpp
@@ +353,5 @@
> +{
> +    if (!emitJumpNoFallthrough(op, jump))
> +        return false;
> +    setJumpTarget(*jump, target);
> +    // Unconditionally create a fallthrough for closing iterators, and as a

style nit: blank line before comment

@@ +366,5 @@
> +{
> +    MOZ_ASSERT(-1 <= jump.offset && jump.offset <= offset());
> +    MOZ_ASSERT(0 <= target.offset && target.offset <= offset());
> +    jsbytecode* pc = code(jump.offset);
> +    jsbytecode* stop = code(-1);

Pretty sure it's undefined behavior in C++ for us to even compute the address code(-1). Instead:

    ptrdiff_t delta;
    for (ptrdiff_t offset = jump.offset; offset != -1; offset += delta) {
        jsbytecode* pc = code(offset);
        ...
    }

@@ +545,5 @@
>  
> +    if (entryJump.offset != -1) {
> +        JumpTarget entry{ offset() };
> +        setJumpTarget(entryJump, entry);
> +    }

Nit: setJumpTarget() is a no-op if `jumps.offset == -1`, right? So no if-statement here.

What guarantees that this will be a jump target instruction?

@@ +557,5 @@
>  
>  void
> +BytecodeEmitter::setContinueTarget(StmtInfoBCE* stmt, JumpTarget target)
> +{
> +    /* Set loop and enclosing "update" offsets, for continue. */

Style nit: please consistently use //

@@ +566,5 @@
> +
> +void
> +BytecodeEmitter::setContinueHere(StmtInfoBCE* stmt)
> +{
> +    /* The next instruction should be a valid jump target. */

and here

@@ +727,5 @@
>  
>  }  // anonymous namespace
>  
>  bool
> +BytecodeEmitter::emitGoto(StmtInfoBCE* toStmt, JumpList* lastp, SrcNoteType noteType)

`lastp` should be renamed.

@@ +976,5 @@
>  
> +    JumpTarget top;
> +    if (!emitJumpTarget(&top))
> +        return false;
> +    pushStatement(stmt, stmtType, top);

Is a jump target necessary here? I don't think we ever actually jump to the top of a block; `top` might even be unused for block scopes.

@@ +1001,4 @@
>      if (!innermostStmt()->isTrying()) {
> +        if (brk.offset == -1 && !emitJumpTarget(&brk))
> +            return false;
> +        setJumpTarget(innermostStmt()->breaks, brk);

Use emitJumpHere() instead, if you can.

@@ +3323,4 @@
>      if (switchOp == JSOP_CONDSWITCH) {
>          unsigned caseNoteIndex;
>          bool beforeCases = true;
> +        JumpList prevCase;

The way this is used is pretty weird and needs a comment.

In particular please mention that the offsets of the JSOP_CASE instructions that need to be patched are stored in the parse nodes, not this JumpList.

@@ +3384,4 @@
>              return false;
>      } else {
>          MOZ_ASSERT(switchOp == JSOP_TABLESWITCH);
> +        // skip default offset

blank line before comment

@@ +3420,5 @@
> +        if (switchOp == JSOP_CONDSWITCH && !caseNode->isDefault()) {
> +            JumpList caseCond;
> +            // The case offset got saved in the caseNode structure after
> +            // emitting the JSOP_CASE jump instruction above.
> +            caseCond.offset = caseNode->offset();

Style nit: instead,

    if (...) {
        // The case offset ...
        // ...
        JumpList caseCond = { caseNode->offset() };
        ...
    }

@@ +3444,5 @@
>  
>      if (!hasDefault) {
>          // If no default case, offset for default is to end of switch.
> +        if (!emitJumpTarget(&defaultOffset))
> +            return false;

This emits 2 jump targets for any switch that has no default case, right?

@@ +5366,5 @@
>      if (!emitTree(pn->pn_kid1))
>          return false;
> +    JumpTarget top;
> +    if (!emitJumpTarget(&top))
> +        return false;

Is a jump target necessary here (at the top of an if-statement)?

@@ +5918,5 @@
>      if (!emit1(JSOP_POP))
>          return false;
>  
> +    JumpTarget endIter{ offset() };
> +    if (!tryNoteList.append(JSTRY_FOR_IN, this->stackDepth, top.offset, endIter.offset))

Here I think you could leave it how it was: just offset() for that last argument, and don't add the local variable endIter.

@@ +6636,5 @@
>  
>      LoopStmtInfo stmtInfo(cx);
>      pushLoopStatement(&stmtInfo, StmtType::DO_LOOP, top);
>  
> +    JumpList skipToEntry;

Nit: A better name would be `empty`.

@@ +7687,5 @@
>              return false;
>  
>          if (optCodeEmitted)
> +            if (!emitJumpHere(jmp))
> +                return false;

Style nit: add curly braces around the outer if

@@ +7923,5 @@
>      /* Emit code for the labeled statement. */
>      StmtInfoBCE stmtInfo(cx);
> +    JumpTarget stmtStart;
> +    if (!emitJumpTarget(&stmtStart))
> +        return false;

Is a jump target necessary here?

::: js/src/frontend/BytecodeEmitter.h
@@ +131,5 @@
>  
> +// List of jump instructions. This structure stores the offset of the last
> +// emitted jump instruction, while the jump instruction offset stores the
> +// relative offset of the next jump instruction in the list.
> +struct JumpList {

The comment should say

// Linked list of jump instructions that need to be patched.
// The linked list is stored in the bytes of the incomplete bytecode that will
// be patched, so no extra memory is needed, and patching the instructions
// destroys the list.

and I think instead of an example (as shu suggested) you could make push() and patchAll()
methods of this class:

    // Add a jump instruction to the list.
    void push(bytecode* code, ptrdiff_t newJump) {
        SET_JUMP_OFFSET(&bytecode[newJump], offset - newJump);
        offset = newJump;
    }

    // Patch all jump instructions in this list to jump to `target`.
    // This clobbers the list.
    void patchAll(bytecode* code, JumpTarget target) {
        ptrdiff_t delta;
        for (ptrdiff_t offset = this->offset; offset != -1; offset += delta) {
            ...
        }
    }

(You could almost make offset private, but I think it's not quite worth it.)

It might also be nice to have an assertion that we don't try to do anything to a JumpList after patching it once, but I won't insist.

@@ +138,5 @@
> +    ptrdiff_t offset;
> +};
> +
> +// Jump target used to patch all the jumps in the JumpList.
> +struct JumpTarget {

I don't understand the comment here. It should be something like

    // Offset of a JSOP_JUMPTARGET instruction, used for patching jump instructions.

right?

Comment 39

[Jason Orendorff [:jorendorff]]

Comment on attachment 8745344 [details] [diff] [review]
part 5 - Add a simple way to walk the bytecode until we find a consumer.

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

::: js/src/jsopcode.cpp
@@ +153,5 @@
> +        pc = GetNextPc(pc);
> +        def -= StackUses(nullptr, pc);
> +        if (def <= 0)
> +            return pc;
> +        def += StackDefs(nullptr, pc);

Not sure how this is meant to be used. Can you assert that we don't scan past any jumps?

::: js/src/jsopcode.h
@@ +513,5 @@
>  extern unsigned
>  StackDefs(JSScript* script, jsbytecode* pc);
>  
> +jsbytecode*
> +FindConsumerPc(jsbytecode* pc, int32_t def = 1);

// Scan forward in bytecode from pc, looking for the place where a particular value in the
// operand stack is used or discarded. The stack slot in question is the one at
// `REGS.sp[-def]` after running the instruction at pc.
//
// Returns a pointer to the instruction that consumes the value.
// The caller must know that the value is consumed before the next branch in the code.

::: js/src/vm/NativeObject.cpp
@@ +1916,5 @@
>      if (JSID_IS_ATOM(id, cx->names().iteratorIntrinsic))
>          return true;
>  
>      // Do not warn about tests like (obj[prop] == undefined).
> +    pc = FindConsumerPc(pc);

This is fixing a bug, right? Add a test.

Comment 40

[Jason Orendorff [:jorendorff]]

Comment on attachment 8745351 [details] [diff] [review]
part 7 - Add a no-op bytecode to filter out branches results from the decompiler.

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

Nice! If this changes behavior, it needs a test.

::: js/src/jsopcode.cpp
@@ +273,3 @@
>  
>          // When control-flow merges, intersect the stacks, marking slots that
>          // are defined by different offsets with the UINT32_MAX sentinel.

Change the comment form "the UINT32_MAX sentinel" to "the UnknownOffset sentinel".

@@ +297,5 @@
>  
> +    // Use a struct instead of an enum class to avoid casting the enumerated
> +    // value.
> +    struct SpecialOffsets {
> +        static const uint32_t UnknowOffset = UINT32_MAX;

typo: it should be "UnknownOffset"

Comment 41

[Brian Hackett [Laid off!]]

Comment on attachment 8745359 [details] [diff] [review]
part 10 - Only increment code coverage counters on jump targets.

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

I guess I didn't realize that script profiling is now intended to be an always-on thing.  It would be good to know how much that impacts the engine's memory usage --- the old profiling stuff wasn't written with memory usage in mind, it was intended for debugging applications like code coverage analysis and so forth.  Even though that has improved by only tracking coverage for jump targets, it still seems like it will be substantial, and most of it of it will be wasted on scripts that never get hot enough to even baseline compile.  To that end, it would be really nice if Ion PGO profiling was only done in baseline.

Comment 42

[Nicolas B. Pierron [:nbp]]

(In reply to Jason Orendorff [:jorendorff] from comment #38)
> Comment on attachment 8745339 [details] [diff] [review]
> part 4 - BytecodeEmitter: Distinguish offsets based on their purposes.

Thanks for all the feedback, I will submit a newer version to get shu's review as well.

> @@ +545,5 @@
> >  
> > +    if (entryJump.offset != -1) {
> > +        JumpTarget entry{ offset() };
> > +        setJumpTarget(entryJump, entry);
> > +    }
> 
> Nit: setJumpTarget() is a no-op if `jumps.offset == -1`, right? So no
> if-statement here.
> 
> What guarantees that this will be a jump target instruction?

This function also emit a JSOP_LOOPENTRY right after, the function BytecodeIsJumpTarget added in part 8 is used to ensure that both when we patch the jumps target, and also as part of the validation of the JSScript.

We don't know if this would be a jump target or not, but this is likely, and as mentioned 

> @@ +976,5 @@
> >  
> > +    JumpTarget top;
> > +    if (!emitJumpTarget(&top))
> > +        return false;
> > +    pushStatement(stmt, stmtType, top);
> 
> Is a jump target necessary here? I don't think we ever actually jump to the
> top of a block; `top` might even be unused for block scopes.


> @@ +1001,4 @@
> >      if (!innermostStmt()->isTrying()) {
> > +        if (brk.offset == -1 && !emitJumpTarget(&brk))
> > +            return false;
> > +        setJumpTarget(innermostStmt()->breaks, brk);
> 
> Use emitJumpHere() instead, if you can.

Note, I deliberately did not use it here because I wanted to remove the double  jump-target  opcode that were present at the end of the loops.

I guess what I could do, is remove the brk argument and alias consecutive jump targets to the same opcode.  In which case we can use emitJumpHere at this location. (as mentioned in the last paragraph of comment 36)

I will keep this code as-is in this patch and make a new patch on top of the current patch series to alias consecutive jump targets.

> @@ +3323,4 @@
> >      if (switchOp == JSOP_CONDSWITCH) {
> >          unsigned caseNoteIndex;
> >          bool beforeCases = true;
> > +        JumpList prevCase;
> 
> The way this is used is pretty weird and needs a comment.
> 
> In particular please mention that the offsets of the JSOP_CASE instructions
> that need to be patched are stored in the parse nodes, not this JumpList.

I rename this variable "ptrdiff_t lastCaseOffset", and moved the "JumpList caseJump" inside the loop. This distinguish the 2 use cases, one which is made to register the offsets of the JSOP_CASE in the source notes, and the other which is made to record a new JumpList for each case node.

I also added a comment above the JumpList to explain that we are currently producing the case clauses, but not the case body.

> @@ +3420,5 @@
> > +        if (switchOp == JSOP_CONDSWITCH && !caseNode->isDefault()) {
> > +            JumpList caseCond;
> > +            // The case offset got saved in the caseNode structure after
> > +            // emitting the JSOP_CASE jump instruction above.
> > +            caseCond.offset = caseNode->offset();
> 
> Style nit: instead,
> 
>     if (...) {
>         // The case offset ...
>         // ...
>         JumpList caseCond = { caseNode->offset() };
>         ...
>     }

This syntax is not valid for JumpList, as there is a default constructor, and no constructor with a single argument.  I honestly think that this is a desirable feature to refuse such syntax, as JumpList and JumpTarget are similar, and because the JumpList default constructor ensure that the list is empty.

So I will move the comment above, but keep the

    caseCond.offset = …;

As I think this is the exception, which is breaking the common/safe rule.

> @@ +3444,5 @@
> >  
> >      if (!hasDefault) {
> >          // If no default case, offset for default is to end of switch.
> > +        if (!emitJumpTarget(&defaultOffset))
> > +            return false;
> 
> This emits 2 jump targets for any switch that has no default case, right?

Indeed, either if the last case body is empty, or if it has a break statement.
The optimization that I mentioned above, about aliasing consecutive JumpTarget, should get rid of this issue.

> @@ +5366,5 @@
> >      if (!emitTree(pn->pn_kid1))
> >          return false;
> > +    JumpTarget top;
> > +    if (!emitJumpTarget(&top))
> > +        return false;
> 
> Is a jump target necessary here (at the top of an if-statement)?

No, I replaced it by 

  JumpTarget top{ offset() };

but I have to admit that I ignore why this has to be set to the current offset.

> @@ +7923,5 @@
> >      /* Emit code for the labeled statement. */
> >      StmtInfoBCE stmtInfo(cx);
> > +    JumpTarget stmtStart;
> > +    if (!emitJumpTarget(&stmtStart))
> > +        return false;
> 
> Is a jump target necessary here?

No, Removing it works fine.

> ::: js/src/frontend/BytecodeEmitter.h
> @@ +131,5 @@
> >  
> > +// List of jump instructions. This structure stores the offset of the last
> > +// emitted jump instruction, while the jump instruction offset stores the
> > +// relative offset of the next jump instruction in the list.
> > +struct JumpList {
> 
> The comment should say
> 
> // Linked list of jump instructions that need to be patched.
> // The linked list is stored in the bytes of the incomplete bytecode that
> will
> // be patched, so no extra memory is needed, and patching the instructions
> // destroys the list.
> 
> and I think instead of an example (as shu suggested) you could make push()
> and patchAll()
> methods of this class:
> 
>     void push(bytecode* code, ptrdiff_t newJump) {
>     void patchAll(bytecode* code, JumpTarget target) {

Nice :)

> It might also be nice to have an assertion that we don't try to do anything
> to a JumpList after patching it once, but I won't insist.

Unfortunately, we use the jump offset in many of the source notes, mostly for Ion.
So we cannot poison the offset of the JumpList.

In practice, this code has enough assertion to catch these kind of issues.

> @@ +138,5 @@
> > +    ptrdiff_t offset;
> > +};
> > +
> > +// Jump target used to patch all the jumps in the JumpList.
> > +struct JumpTarget {
> 
> I don't understand the comment here. It should be something like
> 
>     // Offset of a JSOP_JUMPTARGET instruction, used for patching jump
> instructions.
> 
> right?

Correct.

Comment 43

[Jason Orendorff [:jorendorff]]

All of that sounds good to me.

Comment 44

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - BytecodeEmitter: Distinguish offsets based on their purposes. r=jorendorff r?shu

Comment 45

[Nicolas B. Pierron [:nbp]]

(In reply to Jason Orendorff [:jorendorff] from comment #39)
> Comment on attachment 8745344 [details] [diff] [review]
> part 5 - Add a simple way to walk the bytecode until we find a consumer.
> 
> Review of attachment 8745344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jsopcode.cpp
> @@ +153,5 @@
> > +        pc = GetNextPc(pc);
> > +        def -= StackUses(nullptr, pc);
> > +        if (def <= 0)
> > +            return pc;
> > +        def += StackDefs(nullptr, pc);
> 
> Not sure how this is meant to be used. Can you assert that we don't scan
> past any jumps?

Good call.

> ::: js/src/vm/NativeObject.cpp
> @@ +1916,5 @@
> >      if (JSID_IS_ATOM(id, cx->names().iteratorIntrinsic))
> >          return true;
> >  
> >      // Do not warn about tests like (obj[prop] == undefined).
> > +    pc = FindConsumerPc(pc);
> 
> This is fixing a bug, right? Add a test.

This is fixing a bug which is already present when running the test suite with the addition of jump target opcodes, in part 8.

(In reply to Jason Orendorff [:jorendorff] from comment #40)
> Comment on attachment 8745351 [details] [diff] [review]
> part 7 - Add a no-op bytecode to filter out branches results from the
> decompiler.
> 
> Nice! If this changes behavior, it needs a test.

Same thing, this change is made to keep the original behaviour (after landing part 8) which is already checked by multiple tests in the test suite.  So, there is no need for additional tests.

(In reply to Jason Orendorff [:jorendorff] from comment #4)
> Comment on attachment 8737885 [details] [diff] [review]
> part 1.5 - Add JSScript::assertValidJumpTargets.
> 
> ::: js/src/jsscript.cpp
> @@ +3124,5 @@
> > +JSScript::assertValidJumpTargets() const
> > +{
> > +    jsbytecode* end = codeEnd();
> > +    jsbytecode* mainEntry = main();
> > +    for (jsbytecode* pc = code(); pc != end; pc = GetNextPc(pc)) {
> 
> There's a BytecodeRange class in jsopcodeinlines.h for iterating over
> bytecodes. Maybe it's not worth using here, though.

I think it would be interesting to unify all of them at once, espcecially since the BytecodeParser and the FlowGraphSummary classes are quite similar in functions.

I will try to do that later as a follow-up.

Comment 46

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8745344 [details] [diff] [review]
part 5 - Add a simple way to walk the bytecode until we find a consumer.

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

Since the removal of the additional JumpTarget emitted for pushStatement, no JSOP_JUMPTARGET opcode are added in-between the getprop and the ifeq opcodes.

Thus, this patch should no longer be necessary.

Comment 47

[Shu-yu Guo [:shu]]

Comment on attachment 8752228 [details] [diff] [review]
part 4 - BytecodeEmitter: Distinguish offsets based on their purposes. r=jorendorff r?shu

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

New names are much clearer to me, looks good.

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13a71864396d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c922e32b439f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e89325dee16d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e14cb06d99a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3cc52aba3fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/0feaa758b669
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b1daf5127d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae61f4fcb292
https://hg.mozilla.org/integration/mozilla-inbound/rev/4bb616c2f912
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3912b7cd8ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e29695fee87

Comment 49

[Nicolas B. Pierron [:nbp]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=496da299b91c88e9f5c8ec8a5d9cbe1aefcba45d

Comment 50

[Nicolas B. Pierron [:nbp]]

Attachment: part 10.1 - Baseline: Increment counter located at the entry point of a script.

This patch does the same thing as done by the COUNT_COVERAGE_MAIN macro in
the interpreter.

This is ciritcal for performance as Bug 1263645 disables the counters from
the interpreter, which implies that all entry counters are now zero, which
caused all inlined bodies to be removed by the branch pruning optimization.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13a71864396d
https://hg.mozilla.org/mozilla-central/rev/c922e32b439f
https://hg.mozilla.org/mozilla-central/rev/e89325dee16d
https://hg.mozilla.org/mozilla-central/rev/e14cb06d99a7
https://hg.mozilla.org/mozilla-central/rev/b3cc52aba3fb
https://hg.mozilla.org/mozilla-central/rev/0feaa758b669
https://hg.mozilla.org/mozilla-central/rev/2b1daf5127d4
https://hg.mozilla.org/mozilla-central/rev/ae61f4fcb292
https://hg.mozilla.org/mozilla-central/rev/4bb616c2f912
https://hg.mozilla.org/mozilla-central/rev/d3912b7cd8ea
https://hg.mozilla.org/mozilla-central/rev/2e29695fee87

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c2b51f5ad843

Comment 53

[Carsten Book [:Tomcat]]

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