804676 - Remove dependence of Ion compilation on ScriptAnalysis::analyzeTypes

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

This is the function which does script-local type propagation, and is basically the static side of type inference.  The work it does could be changed to a type specialization component of IonMonkey, which would have several advantages.  A preliminary SSA pass would not be needed, and much of the memory allocated by type inference and stored in analysis-temporary would not live past compilation at all.  Constraints on object property type sets would still be necessary, and memory usage would effectively always be that which we currently have after purging analysis memory.

This would require removing JM+TI, which depends on inferred stack types, and we would instead have a baseline JIT which accumulates type information for use during Ion compilation (not yet filed).  As the MIR is constructed Ion would need to fill in intermediate types for stack values, which is pretty straightforward except for phis passed over loop backedges for locals/args modified within the loop.  For this I think it would be nice if the baseline JIT just tracked the observed types at loop entry of variables modified in the loop, which would avoid needing any fixpointing during MIR construction.  Not sure yet if that is the best option.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: prototype (9a5191dfae8d)

Prototype that works on small examples.  Removes TypeOracle and structures IonBuilder so that instead of running analyzeTypes() it keeps track of intermediate types for MIR nodes and dynamically figures out whether type barriers are needed when doing property reads, writes and calls.  For better robustness/correctness this needs to merge incoming types better for phi nodes (it just uses the type of the first input), which depends on having better type information for locals at loop heads (see comment 0).  Otherwise though this approach should be basically sound.

Comment 2

[Shu-yu Guo [:shu]]

Another important advantage of this approach is to make inlining of self-hosted functions more viable. Right now any higher-ordered method on a collection class that we self-host (e.g. Array.prototype.map or the ParallelArray stuff) will eventually become megamorphic at:

 1) The callsite of the callback.
 2) The read/writes of the collection itself.

This is less than desirable, obviously.

Comment 4

[Brendan Eich [:brendan]]

Who owns this? Blocking a ParallelArray blocker.

/be

Comment 5

[Brian Hackett [Laid off!]]

I do.  This is waiting for the baseline compiler to merge, should not take terribly long after that.

Comment 6

[Nicholas Nethercote [inactive]]

I'm upgrading this to MemShrink:P1:  "much of the memory allocated by type inference and stored in analysis-temporary would not live past compilation at all... memory usage would effectively always be that which we currently have after purging analysis memory."  That memory consumption can be large, and it's spiky, which means it's likely to cause OOMs when memory is tight.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: WIP (061b9318815b)

WIP, passes jit-tests --ion but still some gaps --- mainly, phis always have value type and performance is otherwise untested.

Comment 8

[Brian Hackett [Laid off!]]

Attachment: WIP

WIP with better handling for phis.  Non-loop phis are assigned type info based on merging the types of their inputs when their basic block is processed, simple to do because of the rpo graph traversal.  For loops, two things:

1. A simple scan of the loop body is done to see what new types might be assigned to variables in the loop body, which are used to populate the variable's potential types at the loop head.  This doesn't work in all cases, e.g. for complex assignments between vars or when the variable is assigned values in code that has never run (but whose behavior can be deduced by Ion).  In these cases we fall back to option 2.

2. If the loop backedge is reached and variables have types flowing back to the header which are not accounted for in the header's phis, the new types are preserved in the phis and the MIR for the rest of the loop body is thrown away and regenerated.  This happens < 10 times on SS.  This is a fixpointing operation, but since the old, potentially faulty MIR is not preserved this approach should still behave pretty cleanly.

Comment 9

[Brian Hackett [Laid off!]]

Attachment: WIP

Yet another WIP.  This is at parity with trunk on ss, but regresses kraken a bit and several v8/octane benchmarks a lot, so still needs more work to track down the differences there.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: patch (061b9318815b)

This patch still regresses octane and kraken by 1-2%, but from what I've looked at this is due to new issues exposed by expected changes in type information: calls can be inlined now without needing to worry about type barriers at all, and the extra types in the callee can lead to bad codegen choices being made.  Fixing these is beyond the scope of this patch, plus this patch is already pretty huge, so I think landing this one on the IonMonkey branch and then stacking on whatever other optimizations are necessary should be fine.

Attaching the whole thing here for fuzzing, will split it up for review shortly.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: control flow changes

Changes related to tracking types through a script's control flow.

- When we initially start processing a block we need to know the types that will be used for its phis.

- If we hit a backedge and find new types for loop phis we need to throw the loop body away and start over.

- Add an analysis to pick up types for loop-modified variables in many cases without requiring reanalysis of the loop body.

Comment 12

[Brian Hackett [Laid off!]]

Attachment: osr changes

When unboxing OSR values, use type information merged from the state at the head of the loop with the actual types for variables in the OSR frame.  This might have performance problems with off thread compilation as the OSR variable could still change type in the loop body between the points when compilation is triggered and when it completes, leading to unrecoverable bailouts.  I haven't tested if this is an actual problem yet, and it can be fixed by restructuring the code around loops some (we don't analyze loop bodies for new types until after creating the OSR preheader).

Comment 13

[Brian Hackett [Laid off!]]

Attachment: Add MCallInitElementArray

INITELEM_ARRAY unfortunately needs stub calls in the cases when the initializer has never executed and the array's type information is incomplete.  This used to be done directly via constraints added in analyzeTypes, but we now no longer modify type information until the associated side effect actually occurs.

Comment 14

[Brian Hackett [Laid off!]]

Attachment: remove TypeScript::CheckBytecode

CheckBytecode has been great for surfacing inference and compilation related bugs in the past (more than one hundred [infer failure] ... bugs in my inbox) but the property it checks, that inferred type information overapproximates what can actually occur, is no longer guaranteed.  I think the only way this property breaks right now is that Ion ignores callee argument type sets while inlining, so can generate values in the callee that have never been seen before there.  Once the interpreter no longer monitors type information then this property will fail to hold even more often.  (Plus when analyzeTypes is actually gone, the information which these checks query will no longer exist.)

Comment 15

[Brian Hackett [Laid off!]]

Attachment: remove TypeOracle

TypeOracle's functionality has been folded into IonBuilder itself (next patch) and can be removed.

Comment 16

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: debug and opt stacks

With the patch in comment 10:

(function() {
    for (var x = 0; x < 9; x++) {
        for (var y = 0; y < 9; y++) {}
    }
})()

causes Assertion failure: hasBaselineScript(), at ion/BaselineInspector.h with --no-baseline --no-jm in 64-bit debug shell builds and causes a probable null crash in 64-bit opt shell builds at js::ion::BaselineScript::icEntryFromPCOffset.

Comment 17

[Brian Hackett [Laid off!]]

Attachment: everything else

The remaining changes are to IonBuilder and the MIR.  Type information, including type sets, is attached to MIR nodes and used throughout IonBuilder for deciding what nodes to generate for each op.

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 735378 [details] [diff] [review]
patch (061b9318815b)

feedback-, please see comment 16.

Comment 19

[Brian Hackett [Laid off!]]

Renaming this bug.  After these patches Ion compilation no longer requires analyzeTypes to have been run on the scripts being processed, but there are two remaining users of analyzeTypes --- JM+TI will use it until JM itself is removed, and AnalyzeNewScriptProperties will need to be changed to use MIR instead in another bug.

Comment 20

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

As per Brian's request over IRC, I moved comment 16 out to its own bug 860060.

Comment 21

[Nicholas Nethercote [inactive]]

> After these patches Ion compilation no longer requires
> analyzeTypes to have been run on the scripts being processed, but there are
> two remaining users of analyzeTypes --- JM+TI will use it until JM itself is
> removed, and AnalyzeNewScriptProperties will need to be changed to use MIR
> instead in another bug.

When bugs are file for those steps, the MemShrink:P1 tag should be moved... probably to the "remove JM" bug.

Comment 22

[Nicolas B. Pierron [:nbp]]

Comment on attachment 735378 [details] [diff] [review]
patch (061b9318815b)

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

::: js/src/ion/IonBuilder.cpp
@@ +1505,5 @@
> +        // present at the original loop header, then uses of the variables'
> +        // phis may have generated incorrect nodes. The new types have been
> +        // incorporated into the header phis, so remove all blocks for the
> +        // loop body and restart with the new types.
> +        return restartLoop(state);

Use AbortReason_UnstableType here, and add a counter to prevent too many re-entry in this path, such as if we have a really unstable loop type, we don't cycle on it.

A counter might also help to detect under-approximations.

@@ -2997,5 @@
> -    if (argsBarrier) {
> -        if (!thisCall.init(callInfo))
> -            return false;
> -
> -        addTypeBarrier(0, thisCall, oracle.thisTypeSet(calleeScript));

Don't we have argument type sets that we can match against ?

@@ +4202,5 @@
>      // argc+1: MPassArg(JSFunction *), the 'f' in |f.call()|, in |this| position.
>      // argc+2: The native 'call' function.
>  
> +    int calleeDepth = -((int)argc + 2);
> +    int funcDepth = -((int)argc + 1);

Thank you !!!

::: js/src/ion/MIRGraph.cpp
@@ +815,5 @@
>  
> +    if (hadTypeChange) {
> +        for (MPhiIterator phi = phisBegin(); phi != phisEnd(); phi++)
> +            phi->removeOperand(phi->numOperands() - 1);
> +        return AbortReason_Disable;

Add a new AbortReason, such as AbortReason_UnstableType

Comment 23

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Nicholas Nethercote [:njn] from comment #21)
> > After these patches Ion compilation no longer requires
> > analyzeTypes to have been run on the scripts being processed, but there are
> > two remaining users of analyzeTypes --- JM+TI will use it until JM itself is
> > removed, and AnalyzeNewScriptProperties will need to be changed to use MIR
> > instead in another bug.
> 
> When bugs are file for those steps, the MemShrink:P1 tag should be moved...
> probably to the "remove JM" bug.

The "remove JM" bug is bug 857845.

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 735433 [details] [diff] [review]
Add MCallInitElementArray

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

::: js/src/ion/CodeGenerator.cpp
@@ +4712,5 @@
> +{
> +    pushArg(ToValue(lir, LCallInitElementArray::Value));
> +    pushArg(Imm32(lir->mir()->index()));
> +    pushArg(ToRegister(lir->getOperand(0)));
> +    pushArg(ImmWord(lir->mir()->pc()));

Nit: you can use lir->mir()->resumePoint()->pc() to get the pc so that you don't have to store it in the MIR.

Comment 25

[Brian Hackett [Laid off!]]

(In reply to Nicholas Nethercote [:njn] from comment #21)
> > After these patches Ion compilation no longer requires
> > analyzeTypes to have been run on the scripts being processed, but there are
> > two remaining users of analyzeTypes --- JM+TI will use it until JM itself is
> > removed, and AnalyzeNewScriptProperties will need to be changed to use MIR
> > instead in another bug.
> 
> When bugs are file for those steps, the MemShrink:P1 tag should be moved...
> probably to the "remove JM" bug.

While analyzeTypes won't be gone from the code base until JM is gone and AnalyzeNewScriptProperties is fixed up, this bug removes the main outlet via which we currently allocate type constraints and intermediate type sets for scripts in a way that persists until GC.  So this bug should probably keep the MemShrink:P1.

Comment 26

[Brian Hackett [Laid off!]]

(In reply to Nicolas B. Pierron [:nbp] from comment #22)
> @@ -2997,5 @@
> > -    if (argsBarrier) {
> > -        if (!thisCall.init(callInfo))
> > -            return false;
> > -
> > -        addTypeBarrier(0, thisCall, oracle.thisTypeSet(calleeScript));
> 
> Don't we have argument type sets that we can match against ?

With this change, Ion is free to inline calls without paying attention to the argument type sets of the scripts it is inlining.  Only property type sets attached to type objects are now required to correctly model the types that can appear in the program.  All other type sets --- argument, return, bytecode type sets --- are hints that may or may not capture all types that have been realized.  IonBuilder is only required to maintain a representation of types that is consistent both internally and with the property type sets; it is free to ignore the argument type sets when inlining and does not need to insert type barriers or anything.

Now, sometimes having barriers for the argument type sets is beneficial.  From what I've seen, the cause for the remaining slowdown vs. trunk is due to this.  One example based on a case I saw on crypto (and fixed):

function intAt(s, i) {
  s.charCodeAt(i);
}

function bnpFromString(s) {
  ... intAt(s, i);
}

's' in bnpFromString is an int or an array, but it just so happens that intAt is only called with a string; there's no way for the compiler to prove that only strings will flow to the intAt due to typeof etc.  With this patch we were inlining the intAt call and compiling charCodeAt with a string|object input and making a VM call, which will be a good deal slower than either not inlining (pre bug 796114) or inlining the call with a barrier at the call site (post bug 796114).

I fixed this in the patch by allowing s.charCodeAt to be optimized for string|object if the property being searched for is on String.prototype (in which case it is presumably going to be a string).  In general though this sort of fix is unprincipled, and what we should be doing is using baseline information to see what types have actually flowed through the operation (not just what types *could* flow there per IonBuilder's type model) and insert unboxes and similar guards as needed.

This approach is preferable to using the argument type sets when inlining, as the latter only inserts guards at specific points rather than anywhere in the CFG; if bnpFromString had manually inlined the s.charCodeAt bit then trunk would need to make a VM call on the access.

Comment 27

[Christian Holler (:decoder)]

Fyi, I just started testing this patch :)

Comment 28

[Christian Holler (:decoder)]

Comment on attachment 735378 [details] [diff] [review]
patch (061b9318815b)

function test() {
  var result;
  for (var i in scripts) {
    try { result = scripts[i].exec(); }
    catch (e) { result = e; }
  }
}
test();

Causes "Assertion failure: index < stackPosition_, at js/src/ion/MIRGraph.cpp:335" using options --ion-eager.



function printStatus (msg) {
  msg = msg.toString();
  for (var i=0; i<lines.length; i++)
    print (STATUS + lines[i]);
}
  try {
new Function("\
  function Bug() { printStatus (e) }\
  var actual = (new Bug instanceof Bug);\
")();
} catch(exc0) {}
new Function("printStatus (1 & 1);")();

Causes "Assertion failure: !types->unknown(), at js/src/ion/IonMacroAssembler.cpp:61" using options --ion-eager.



var gTestcases = new Array();
function TestCase(n, d, e, a)
this.passed = getTestCaseResult(e, a);
function getTestCaseResult(expected, actual) {
    if (TestCase    )
        return gTestcases;
}
new TestCase(null, 0,eval("x = new Boolean(true); x.charCodeAt=String.prototype.charCodeAt;x.charCodeAt(0)") );
new TestCase('c');

Causes "Assertion failure: alloc->toUse()->virtualRegister() < numVregs_, at js/src/ion/LiveRangeAllocator.h:469" using options --ion-eager on a 32-bit build.



function x()
    [null].some(x())
x();

Causes "Assertion failure: hasScript(), at ../jsfun.h:238" using options --ion-eager.



var obj = new Object();
for (var i = 0; i < 100; i++) {
    obj['-1'] = obj;
    assertEq(obj['-1'] == null, false);
}

Causes "Assertion failure: id == IdToTypeId(id), at ../jsinferinlines.h:1607" using options --ion-eager.


That's enough for now I guess ;) I'll switch to fuzzing one of the other patches that currently require feedback. Whenever this is ready again for fuzzing and you need more of it, please let me know :)

Comment 29

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

jsfunfuzz also hit the "Assertion failure: !types->unknown()," and "Assertion failure: hasScript()," bugs, fwiw.

Comment 30

[Brian Hackett [Laid off!]]

Pushing to the IonMonkey branch for further fuzz bug and perf work while review is pending:

https://hg.mozilla.org/projects/ionmonkey/rev/ee14945b452c

Comment 31

[Brian Hackett [Laid off!]]

Attachment: fuzz bug fixes

Fix fuzz bugs reported so far.  Gary, decoder, if you can start fuzzing again on this soon that would be great.  Everything is now on the IonMonkey repo.

https://hg.mozilla.org/projects/ionmonkey/rev/79f78c194329

Comment 32

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 735430 [details] [diff] [review]
control flow changes

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

For some reason splinter is busted for a few files in this patch, so just submitting comments now before I jump to the old fashioned view.

::: js/src/ion/IonBuilder.h
@@ +515,5 @@
>                             Vector<MDefinition *, 8, IonAllocPolicy> &retvalDefns);
>  
> +    types::StackTypeSet *cloneTypeSet(types::StackTypeSet *types);
> +
> +    void setCurrent(MBasicBlock *block) {

With this, is it illegal to ever set current_ without going through setCurrent?

::: js/src/ion/MIR.cpp
@@ +677,5 @@
> +        // that could come in via loop backedges.
> +        start = 0;
> +    } else {
> +        setResultType(inputs_[0].producer()->type());
> +        setResultTypeSet(inputs_[0].producer()->resultTypeSet());

Does getOperand(0) work here?

@@ +705,5 @@
> +
> +        MergeTypes(&resultType, &resultTypeSet, type, typeSet);
> +
> +        setResultType(resultType);
> +        setResultTypeSet(resultTypeSet);

this pattern comes up in a few places - if it would be easier, feel free to make resultType_ protected or add a mergeTypes helper to MDefinition or something.

::: js/src/ion/MIR.h
@@ +3287,5 @@
>      INSTRUCTION_HEADER(Phi)
>      static MPhi *New(uint32_t slot);
>  
>      void setOperand(size_t index, MDefinition *operand) {
> +        // Note: after the initial IonBuilder pass, it is OK to rearrange phi

Could you say "change" instead of "rearrange", to make it clear the order doesn't change?

Comment 33

[Brian Hackett [Laid off!]]

(In reply to David Anderson [:dvander] from comment #32)
> ::: js/src/ion/IonBuilder.h
> @@ +515,5 @@
> >                             Vector<MDefinition *, 8, IonAllocPolicy> &retvalDefns);
> >  
> > +    types::StackTypeSet *cloneTypeSet(types::StackTypeSet *types);
> > +
> > +    void setCurrent(MBasicBlock *block) {
> 
> With this, is it illegal to ever set current_ without going through
> setCurrent?

No, when rewinding loops we need to update current_ without going through setCurrent as the block being updated to is the loop header and has already been visited.  setCurrent could use a better name, maybe setCurrentAndMergePhiTypes.

Comment 34

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 735430 [details] [diff] [review]
control flow changes

With that name change, could we make current_ updates always go through a setter (the simple one being just setCurrent), and put them close together with an explanation when to use which?

The loop restarting idea is clever. In retrospect it makes a lot of sense. My main concern is how much it is gated on analyzeNewLoopTypes heuristics, and potential explosion on loops that contain many other loops. Those seem like solvable problems though, and we won't really know how bad they are until we just put more test cases through the code. Maybe in the interim, we should have a failsafe to stop compilation if we've revisited loops too many times in one IonBuilder. I don't know what a reasonable amount would be though - we'd probably have to construct or find a really pathological test to find out.

>+    if (!pushLoop(state.loop.initialState, state.loop.initialStopAt, header,
>+                  state.loop.loopHead, state.loop.initialPc,
>+                  state.loop.bodyStart, state.loop.bodyEnd,
>+                  state.loop.exitpc, state.loop.continuepc))
>+        return ControlStatus_Error;

nit: brace here

Comment 35

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 735432 [details] [diff] [review]
osr changes

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

::: js/src/ion/IonBuilder.cpp
@@ +55,5 @@
>      pc = info->startPC();
> +
> +    if (!script_->hasFreezeConstraints) {
> +        types::TypeScript::AddFreezeConstraints(cx, script_);
> +        script_->hasFreezeConstraints = true;

Can AddFreezeConstraints update the hasFreezeConstraints bit?

Comment 36

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 735442 [details] [diff] [review]
remove TypeOracle

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

I won't miss you, Oracle!

(Coincidentally, I probably said the same thing when we deleted jstracer...)

Comment 37

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 735444 [details] [diff] [review]
everything else

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

Nice! I didn't read the individual opcode cases too closely after a bit, but my impression is that this explicit querying (which was already needed in a bunch of places anyway), is a lot more readable than the random Oracle abstractions we had before.

::: js/src/ion/IonBuilder.cpp
@@ -5411,5 @@
>  
> -// Test the type of values returned by a VM call. This is an optimized version
> -// of calling TypeScript::Monitor inside such stubs.
> -void
> -IonBuilder::monitorResult(MInstruction *ins, types::TypeSet *barrier, types::StackTypeSet *types)

Why is it that monitors aren't needed anymore? And after this patch, is there any use for the MMonitorTypes instruction or its paths in Bailouts.cpp? If not, could we remove all of that as a follow-up?

@@ +5917,5 @@
> +    return types && !types->hasObjectFlags(cx, types::OBJECT_FLAG_NON_PACKED);
> +}
> +
> +bool
> +IonBuilder::elementAccessHasExtraIndexedProperty(MDefinition *obj)

Since all of these "elementAccess" and "propertyRead" functions don't use IonBuilder members except for |cx| would it make sense to make them static non-members? When I see functions in IonBuilder I start to wonder what sort of effect or dependencies they have on the overall state. If they're static it might be clearer that they are helpers and not strictly part of MIR generation.

(IonBuilder.cpp is creeping up to that 10KLoC point too, so having the option to move stuff like this into a separate file is nice.)

@@ +6008,5 @@
> +
> +bool
> +IonBuilder::tryAddWriteBarrier(types::StackTypeSet *objTypes, jsid id, MDefinition **pvalue)
> +{
> +    // Return whether value was modified to include a write barrier on

Here and in the function name, "write barrier" is different from a GC write barrier, right? Could we distinguish this name somehow?

@@ +6011,5 @@
> +{
> +    // Return whether value was modified to include a write barrier on
> +    // objTypes/id --- if executing value does not bail out, then writing it
> +    // to the object will not require changing type information. If value is
> +    // not updated, a VM call must always be performed for the write.

Why is this, exactly? Is it because we expect we'll always be adding to the property's typeset?

@@ +6450,5 @@
>      return resumeAfter(ins);
>  }
>  
> +MIRType
> +IonBuilder::denseNativeElementType(MDefinition *obj)

Another candidate for being static?

@@ +7322,3 @@
>  {
>      JS_ASSERT(*emitted == false);
> +    bool accessGetter = script()->analysis()->getCode(pc).accessGetter;

This pattern appears in a few places, maybe add a helper like: analysis(pc).accessGetter?

Comment 38

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 736781 [details] [diff] [review]
fuzz bug fixes

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

::: js/src/ion/shared/Lowering-shared-inl.h
@@ +403,5 @@
>  VirtualRegisterOfPayload(MDefinition *mir)
>  {
> +    // Type barriers may have box inputs, and pass through their input's vreg.
> +    if (mir->isTypeBarrier())
> +        mir = mir->getOperand(0);

How did this ever work before? Did we never use MTypeBarriers as inputs? Can an MTypeBarrier have an MTypeBarrier as its input?

Mysteriously, redefine() doesn't set PASSTHROUGH, but LBox does. So I don't really know what's going on.

Comment 39

[Sean Stangl [:sstangl]]

Comment on attachment 735444 [details] [diff] [review]
everything else

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

::: js/src/ion/IonBuilder.cpp
@@ +6440,5 @@
>          return abort("Type is not definitely lazy arguments.");
> +
> +    current->pop();
> +    current->pop();
> +    current->pop();

current->popn(3)?

Comment 40

[Brian Hackett [Laid off!]]

(In reply to David Anderson [:dvander] from comment #38)
> How did this ever work before? Did we never use MTypeBarriers as inputs? Can
> an MTypeBarrier have an MTypeBarrier as its input?
> 
> Mysteriously, redefine() doesn't set PASSTHROUGH, but LBox does. So I don't
> really know what's going on.

On trunk, MTypeBarrier is used only for the result of property reads, and care is taken to use boxed versions of these in the type barrier case so that the type barrier's input already has MIRType_Value.  The patches here add a second use of MTypeBarrier, as a type filter when writing properties so that a VM call can be avoided.  In this case, the type barrier's input can be any instruction, and if that instruction is an MBox then VirtualRegisterOfPayload returns some junk value for the MTypeBarrier as it isn't able to special case the wrapped MBox.  What these opcodes are doing is pretty mysterious to me too, it would be nice if this stuff was better encapsulated / documented.

Comment 41

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Brian Hackett (:bhackett) from comment #40)
> (In reply to David Anderson [:dvander] from comment #38)
> > How did this ever work before? Did we never use MTypeBarriers as inputs? Can
> > an MTypeBarrier have an MTypeBarrier as its input?
> > 
> > Mysteriously, redefine() doesn't set PASSTHROUGH, but LBox does. So I don't
> > really know what's going on.
> 
> On trunk, MTypeBarrier is used only for the result of property reads, and
> care is taken to use boxed versions of these in the type barrier case so
> that the type barrier's input already has MIRType_Value.  The patches here
> add a second use of MTypeBarrier, as a type filter when writing properties
> so that a VM call can be avoided.  In this case, the type barrier's input
> can be any instruction, and if that instruction is an MBox then
> VirtualRegisterOfPayload returns some junk value for the MTypeBarrier as it
> isn't able to special case the wrapped MBox.  What these opcodes are doing
> is pretty mysterious to me too, it would be nice if this stuff was better
> encapsulated / documented.

Okay, makes sense. If an MTypeBarrier can have an MTypeBarrier as its input, then should that |if| be a |while|?

Comment 42

[Brian Hackett [Laid off!]]

Address review comments:

https://hg.mozilla.org/projects/ionmonkey/rev/7bc766400b81

Comment 43

[Nicholas Nethercote [inactive]]

Nice.  Do you have any memory measurements?  I'd be interested to see a before vs. after comparison of the "js-main-runtime" tree (near the bottom of about:memory) with a variety of sites open.

Comment 44

[Brian Hackett [Laid off!]]

I haven't had time to measure memory usage, unfortunately.  For a normal browser session the memory consumed due to analyzeTypes has historically been pretty low, it just becomes more relevant with JS intensive stuff like games and when memory is constrained, as with b2g.

There are a few minor things to fix yet, but pushing to inbound and will take care of these on trunk.

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

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee14945b452c
https://hg.mozilla.org/mozilla-central/rev/79f78c194329
https://hg.mozilla.org/mozilla-central/rev/7bc766400b81

Comment 46

[Nicholas Nethercote [inactive]]

As per bug 789892 comment 27, downgrading this from MemShrink:P1 to MemShrink:P2 -- this bug will help in cases where we are Ion-compiling a lot of code, but that's not terribly common.  Bug 865059 will provide the big memory consumption win by delaying TI data-gathering after baseline compilation has happened.