939581 - Don't unconditionally insert a resume point for an MCall and make it possible to apply CSE, DCE, and LICM optimizations to DOM calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Boris Zbarsky [:bzbarsky]]

We seem to unconditionally insert a resume point in makeCall.  I don't think we should for cases when the call is infallible and not effectful.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Only insert a resume point for an MCall in cases when the MCall can fail or might have side effects.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w for ease of review

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8333552 [details] [diff] [review]
Only insert a resume point for an MCall in cases when the MCall can fail or might have side effects.

Hrm.  So I'm not sure this is right.

In particular, having no arg to pass where a string arg is expected is not effectful, but it _is_ fallible.  So the processing of the jitinfo's argTypes needs to be a bit different in the getAliasSet() and isInfallible() cases.

So canceling review requests, but would still like to know whether the general idea makes sense, with isInfallible() fixed to be correct.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8333552 [details] [diff] [review]
Only insert a resume point for an MCall in cases when the MCall can fail or might have side effects.

Per discussion, non-effectful stuff that's fallible in a reliable way doesn't need a resume point.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Factor out MCallDOMNative from MCall.

Please pay special attention to the XXX comments!

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make EliminateDeadCode play nice with MCall and MCallDOMNative.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Don't create resume points for non-effectful methods (which currently just means DOM methods), on the assumption that such methods have no side-effects and throw deterministically based on thisval and arguments.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Talked to Eric a bit; will post patches with his IRC comments addressed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Factor out MCallDOMNative from MCall.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make EliminateDeadCode play nice with MCall and MCallDOMNative.

Comment 11

[Eric Faust [:efaust]]

Comment on attachment 8335608 [details] [diff] [review]
part 1.  Factor out MCallDOMNative from MCall.

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

Looks good. Thanks for doing this. r=me

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8335608 [details] [diff] [review]
part 1.  Factor out MCallDOMNative from MCall.

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

Nice.

::: js/src/jit/MIR.cpp
@@ +663,5 @@
> +    const JSJitInfo* jitInfo = getSingleTarget()->jitInfo();
> +    JS_ASSERT(jitInfo);
> +
> +    JS_ASSERT(jitInfo->aliasSet != JSJitInfo::AliasNone);
> +    if (jitInfo->aliasSet != JSJitInfo::AliasDOMSets || !jitInfo->argTypes)

Nit: Add a short comment here to explain why we need the !argTypes check.

::: js/src/jit/MIR.h
@@ +1906,5 @@
> +    // isCall() to check for calls and all we really want is to overload a few
> +    // virtual things from MCall.
> +protected:
> +    MCallDOMNative(JSFunction *target, uint32_t numActualArgs) :
> +        MCall(target, numActualArgs, false)

Nit: indent "protected:" with two spaces, also move the : to the beginning of the next line while you're here:

    MCallDOMNative(...)
      : MCall(...)
    {

@@ +1913,5 @@
> +
> +    friend MCall *MCall::New(TempAllocator &alloc, JSFunction *target, size_t maxArgc,
> +                             size_t numActualArgs, bool construct, bool isDOMCall);
> +
> +public:

Same here.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 8335612 [details] [diff] [review]
part 2.  Make EliminateDeadCode play nice with MCall and MCallDOMNative.

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

::: js/src/jit/IonAnalysis.cpp
@@ +167,5 @@
>          // Remove unused instructions.
>          for (MInstructionReverseIterator inst = block->rbegin(); inst != block->rend(); ) {
>              if (!inst->isEffectful() && !inst->resumePoint() &&
>                  !inst->hasUses() && !inst->isGuard() &&
>                  !inst->isControlInstruction()) {

Pre-existing nit, but can you move the { to the next line?

Comment 14

[Tom Schuster [:evilpie]]

Comment on attachment 8335608 [details] [diff] [review]
part 1.  Factor out MCallDOMNative from MCall.

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

::: js/src/jit/MIR.cpp
@@ +659,5 @@
> +MCallDOMNative::getAliasSet() const
> +{
> +    JS_ASSERT(getSingleTarget() && getSingleTarget()->isNative());
> +
> +    const JSJitInfo* jitInfo = getSingleTarget()->jitInfo();

*jitInfo

@@ +667,5 @@
> +    if (jitInfo->aliasSet != JSJitInfo::AliasDOMSets || !jitInfo->argTypes)
> +        return AliasSet::Store(AliasSet::Any);
> +
> +    uint32_t argIndex = 0;
> +    for (const JSJitInfo::ArgType* argType = jitInfo->argTypes;

*argType

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Nit: Add a short comment here to explain why we need the !argTypes check.

    // If we don't know anything about the types of our arguments, we have to
    // assume that type-coercions can have side-effects, so we need to alias
    // everything.

Addressed all the other comments.

Comment 16

[Eric Faust [:efaust]]

Comment on attachment 8335612 [details] [diff] [review]
part 2.  Make EliminateDeadCode play nice with MCall and MCallDOMNative.

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

See comment below. Grumble grumble grumble. If Jan is OK with this, I will not stand in the way of landing, but I think a little more thought could be useful in this area.

::: js/src/jit/Lowering.cpp
@@ +419,5 @@
>      // Check MPrepareCall/MCall nesting.
>      JS_ASSERT(prepareCallStack_.popCopy() == call->getPrepareCall());
>  
> +    if (call->isDead()) {
> +        // Nothing else we need to do here, since we've already done freeArguments.

I will begrudgingly agree to this design, I guess.

I don't understand why we can't just DCE the passagrs and the preparecall with the call? It should be their only uses anyway, right? And the resume point captures the stack /before/ popping arguments, so they are not captured there, right? This is just for our internal state, all of which is changed during lowering these things, right? So if we just rm them, then this problem goes away?

Am I missing something? That seems cleaner and more like what we want than decoupling the MIR and LIR graphs...

Comment 17

[Boris Zbarsky [:bzbarsky]]

From what little I understand, if we can in fact nix the passargs and preparecall we should be able to nix the call as well.

We do _something_ like that in UnreachableCodeElimination::removeUnmarkedBlocksAndClearDominators near the end, though it's got some comments about resume points that make it sound like a resume point might in fact capture passargs?

I agree that the setup in this patch is totally nuts in general; it's just all I've been able to come up with given the lack of anyone willing to own up to understanding the actual constraints on the whole preparecall/passargs setup so far.  :(

I definitely think we should file a followup bug to come up with a saner setup here.

Comment 18

[Eric Faust [:efaust]]

Setting needinfo on myself so that I remember to do this tomorrow.

I agree that nobody on the jit inside has really stepped up to figure out how feasible it is to undertake other designs, and that my complaint above was not exceeding well-researched, and I am sorry about that. I also fully admit that the solution in the patch is easily the easiest solution for someone on the outside.

I want to play with this for a few hours tomorrow, and see what I can come up with. Can we hold off landing for just a moment in case that yields anything?

Comment 19

[Boris Zbarsky [:bzbarsky]]

Sure thing.  For one, part 3 still needs reviews.  ;)

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merged to tip

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Experiment in making DOM calls movable, based on bug 952992

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Implement congruentTo for DOM calls as well.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merged on top of bug 95299

The only substantive difference here from the earlier patch version is the change in makeCallHelper to use callInfo.thisArg()->resultTypeSet() instead of hoisting the decl of thisArg higher.  The other option is to hoist thisArg higher but also reintroduce the code that munges thisArg if we change callInfo.thisArg().  Note that the change to callInfo.thisArg() only happens in the constructing case, while we only examine the typeset for DOM purposes in the non-constructing case, so this code really is OK.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8335612 [details] [diff] [review]
part 2.  Make EliminateDeadCode play nice with MCall and MCallDOMNative.

This is no longer needed.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Don't create resume points for non-effectful methods (which currently just means DOM methods), on the assumption that such methods have no side-effects and throw deterministically based on thisval and arguments.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Mark DOM calls as movable as needed and allow them to be CSE'd.

This incidentally fixes a preexisting bug in MCallDOMNative::getAliasSet that could affect methods that are marked [Pure] and take sequence arguments, say.  I have audited our existing [Pure] methods that take non-primitive arguments; they are Node.isEqualNode, Node.compareDocumentPosition, and Node.contains.  All three take Node arguments, so while they can't be movable in the new setup they also can't have artument-conversion side-effects (other than throwing), so it was actually safe to mark them as not write-aliasing things....

Comment 27

[Eric Faust [:efaust]]

Comment on attachment 8351607 [details] [diff] [review]
Part 1 merged on top of bug 95299

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

Good. This seems to carry the plus fine

Comment 28

[Eric Faust [:efaust]]

Comment on attachment 8351608 [details] [diff] [review]
part 2.  Don't create resume points for non-effectful methods (which currently just means DOM methods), on the assumption that such methods have no side-effects and throw deterministically based on thisval and arguments.

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

Yes. I now respect this in light of the alias set information from jitinfo.

Comment 29

[Eric Faust [:efaust]]

Comment on attachment 8351609 [details] [diff] [review]
part 3.  Mark DOM calls as movable as needed and allow them to be CSE'd.

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

Look good. r=me

::: js/src/jit/MIR.cpp
@@ +756,5 @@
> +    if (!congruentIfOperandsEqual(call))
> +        return false;
> +
> +    // The other call had better be movable at this point!
> +    JS_ASSERT(call->isMovable());

I am glad that you didn't hold here with the tradition of return congruentIfOperandsEqual().

This assert puts my mind even more at rest :)

Comment 30

[Eric Faust [:efaust]]

Do we have any more recent perf numbers on these changes, just out of curiosity?

Clearing needinfo on self, as Jan's work on passargs renders it no longer relevant.

Comment 31

[Boris Zbarsky [:bzbarsky]]

> Do we have any more recent perf numbers on these changes, just out of curiosity?

It does _really_ well in microbenchmarks.  e.g. Dromaeo's getAttribute test gets loop-hoisted, as does the test at http://jsperf.com/demo-of-microbenchmark-silliness (See the "Firefox 29" numbers there from my test builds).  And once we can do ion optimizations on document, we'll hoist getElementById too.

For real-world code, unclear.

Comment 32

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/4abc092e62c5
   https://hg.mozilla.org/integration/mozilla-inbound/rev/6badd39e9d6f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/6034450c8684

We'll see what talos apart from dromaeo-dom thinks!

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/b4561c14972d to see if this fixes the apparent permaorange rootanalysis failures that popped up: https://tbpl.mozilla.org/php/getParsedLog.php?id=32537813&tree=Mozilla-Inbound

Comment 34

[Boris Zbarsky [:bzbarsky]]

And it does.  What the heck is going on here?  Those are jit-test failures, and they're totally consistent.  Each time it's:

 TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/basic/testArrayBufferSlice.js | --no-baseline --no-ion
 INFO stderr 2> /builds/slave/m-in_l64-d_sm-ra-0000000000000/src/js/src/jit-test/tests/basic/testArrayBufferSlice.js:8:12 Error: Assertion failed: got 16, expected 43

and neither shu nor I can reproduce locally just running jit-test on either mac64 or linux64.

How could these patches possibly affect a --no-ion run in the first place?  How do I go about running exactly what "r" runs on tinderbox locally?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

Not sure how many of these are relevant outside exact rooting and gcgenerational.

--enable-optimize
--enable-debug
--enable-stdcxx-compat
--enable-ctypes
--enable-trace-malloc
--disable-shared-js
--enable-exact-rooting
--enable-gcgenerational
--enable-threadsafe

Comment 36

[Jan de Mooij [:jandem]]

Boris, do you also see this failure on Try? This has happened to me before, with the ggc build. Green on Try, perma-orange on my (unrelated!) inbound push, but fortunately it was green on the next one but it kept failing intermittently (bug 946939) until it was fixed (bug 948162).

Let me try to reproduce it too, but as you said if it's failing with --no-ion --no-baseline it's unlikely to be caused by your changes.

Comment 37

[Jan de Mooij [:jandem]]

Oh and the r build runs with the JS_GC_ZEAL=7 environment var.

Comment 38

[Boris Zbarsky [:bzbarsky]]

> Boris, do you also see this failure on Try

Working on that.  Hoping I found the right try incantation to get this test to run....

Comment 39

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #38)
> Working on that.  Hoping I found the right try incantation to get this test
> to run....

Just make some minor change to js/src/* to get r/e/ggc builds.

Comment 40

[Boris Zbarsky [:bzbarsky]]

All the try runs I did (on top of current inbound) are green.  See https://tbpl.mozilla.org/?tree=Try&rev=344c6b8cef45

Now what?  :(

Comment 41

[Ryan VanderMeulen [:RyanVM]]

Might as well re-land and see what happens.

Comment 42

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Relanded in 
https://hg.mozilla.org/integration/mozilla-inbound/rev/dce412767964
https://hg.mozilla.org/integration/mozilla-inbound/rev/98a783dfcef5
https://hg.mozilla.org/integration/mozilla-inbound/rev/746f32004ab7

Comment 43

[Boris Zbarsky [:bzbarsky]]

And now it's nice and green.  I don't like this test.  :(

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dce412767964
https://hg.mozilla.org/mozilla-central/rev/98a783dfcef5
https://hg.mozilla.org/mozilla-central/rev/746f32004ab7