909717 - IonMonkey: Typebarrier improvements

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Hannes Verschore [:h4writer]]

Typebarrier could get some improvements. Atm there is TypeBarrier/GuardObject/GuardString/GuardObjectType/... It is my intention to combine those. And also remove the BoxInputPolicy. So TypeBarrier could get typed...

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: bug909717-p1-remove-unneeded-loadptr

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: bug909717-p2-typed-typebarrier

This enables typed typebarrier. Finally found the regression:

//Only contains MTypeBarrier now. No unboxing there.
IonBuilder: MTypeBarrier

//During type analysis the typebarrier gets possible an unbox. This does the type checking part
Apply types: MUnbox[TypeBarrier] + MTypeBarrier

// During lowering MTypeBarrier gets removed (when MUnbox does everything), gets lowered to LBail (when types don't match), get's lowered to LTypeBarrierV (when different types flow through) or to LTypeBarrierO (when we have a specific (type)objects to test)
Lowering: / | LBail | LTypeBarrierV | LTypeBarrierO

This could remove some extra checks. Since before typebarrier always boxed it's input. Running v8 I don't really see an improvement/regression. I will definitely push when awfy is running, to make sure this statement is correct

There are still 2 possible perf improvements left in this patch:
- 1: IonAnalysis has some checks to remove TypeBarriers that check for null/undefined, but where input went through an test removing that case. TypeBarrier gets removed indeed. But the Unbox doesn't get removed anymore. I created code for this, but had some unexplainable additional regression due to this. Need to investigate why. Or make sure this is really a regression.

- 2: When input is typed and typebarrier MIRType_Value, we can remove the extra checks why the typebarrier is MIRType_Value and make it only check the types that can occur depending on the input.

TODO:
- Next: Actually remove GuardObject/GuardString/GuardObjectType/

Comment 3

[Jan de Mooij [:jandem]]

Comment on attachment 799128 [details] [diff] [review]
bug909717-p2-typed-typebarrier

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

Nice, some parts are a lot simpler now! r=me with comments below addressed.

::: js/src/jit-test/tests/jaeger/recompile/bug671943-1.js
@@ -1,2 @@
>  gczeal(2);
> -o1 = Iterator;

We should revert the changes to this test.

::: js/src/jit/CodeGenerator.cpp
@@ +1329,5 @@
>  bool
> +CodeGenerator::visitTypeBarrierO(LTypeBarrierO *lir)
> +{
> +    Register obj = ToRegister(lir->object());
> +    Register scratch = ToTempUnboxRegister(lir->temp());

Maybe we should rename ToTempUnboxRegister, it isn't really for unboxing anything here. Or add a new function and have ToTempUnboxRegister call it too. ToMaybeTempRegister? ToTempRegisterOrInvalid?

::: js/src/jit/IonBuilder.cpp
@@ +4163,3 @@
>      //    have at most a single use.
>      JS_ASSERT_IF(callInfo.fun()->isGetPropertyCache(), !cache->hasUses());
> +    JS_ASSERT_IF(callInfo.fun()->isTypeBarrier(), cache->useCount() == 1);

Nit: cache->hasOneUse()

@@ +6158,5 @@
>          return true;
>  
>      current->pop();
>  
> +    MInstruction *barrier = MTypeBarrier::New(ins, cloneTypeSet(observed));

Nice, this looks a lot simpler :)

::: js/src/jit/IonMacroAssembler.cpp
@@ +111,3 @@
>  
> +    // Note: Some platforms give the same register for obj and scratch.
> +    // Make sure when writing to scratch, the obj register isn't used anymore!

This is only true if you use useRegisterAtStart for the input. Should we do that now?

@@ +117,5 @@
> +    for (unsigned i = 0; i < count; i++) {
> +        if (JSObject *object = types->getSingleObject(i))
> +            branchPtr(Equal, obj, ImmGCPtr(object), matched);
> +        else
> +            hasTypeObjects = true;

getObjectCount() can overapproximate if we use a hash table; see the getObjectCount comment in jsinfer.h This means the else should be

else if (types->getTypeObject(i))

::: js/src/jit/Lowering.cpp
@@ +1837,5 @@
>      // from inside a type barrier test.
>  
>      const types::StackTypeSet *types = ins->resultTypeSet();
>      bool needTemp = !types->unknownObject() && types->getObjectCount() > 0;
>      LDefinition tmp = needTemp ? temp() : tempToUnbox();

Why the tempToUnbox? Shouldn't this be LDefinition::BogusTemp() to avoid an unnecessary temp register on x64?

@@ +1868,5 @@
> +    }
> +
> +    // Handle typebarrier with specific TypeObject/SingleObjects.
> +    if (inputType == MIRType_Object && !types->hasType(types::Type::AnyObjectType()) &&
> +        types->getObjectCount())

Remove the "&& types->getObjectCount()", we want to always bail in this case.

::: js/src/jit/MIR.cpp
@@ +645,5 @@
> +    fprintf(fp, " ");
> +    getOperand(0)->printName(fp);
> +    fprintf(fp, " ");
> +
> +    switch(bailoutKind()) {

I think we should move this switch statement to a "static void PrintBailoutKind(fp, bailoutKind)" helper function and call it here and from MUnbox::printOpcode.

::: js/src/jit/MIR.h
@@ +7572,5 @@
>      virtual bool neverHoist() const {
>          return resultTypeSet()->empty();
>      }
> +
> +    bool certainBail() const {

How about alwaysBails()?

::: js/src/jit/TypePolicy.cpp
@@ +231,5 @@
> +    if (inputType == outputType)
> +        return true;
> +
> +    // Output is a value, currently box the input.
> +    // Optimization: decrease resultTypeSet to only include the inputType.

Is this optimization a TODO? It's a bit confusing because it doesn't match the code that follows it.

@@ +246,5 @@
> +
> +        // We can't unbox a value to null/undefined. So keep output also a value.
> +        if (IsNullOrUndefined(outputType) || outputType == MIRType_Magic) {
> +            ins->setResultType(MIRType_Value);
> +            outputType = MIRType_Value;

Nit: you can remove this line.

Comment 4

[Hannes Verschore [:h4writer]]

Attachment: bug909717-part3

To make sure we don't have a regression. The typebarrier is sure to be infallible. We remove it, but now it is typebarrier + unbox. And the unbox doesn't get removed and still has some code. By making it infallible we match the situation before the patch.

Comment 5

[Hannes Verschore [:h4writer]]

Attachment: bug909717-part3

forget to qref

Comment 6

[Hannes Verschore [:h4writer]]

Comment on attachment 800209 [details] [diff] [review]
bug909717-part3

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

Jan noticed this. So that means when I was benchmarking I was actually benchmarking without this change in. I'm gonna remove review since it is fault and I need to run performance numbers again.

Landed part 1+2
https://hg.mozilla.org/integration/mozilla-inbound/rev/94d54fe84c77

::: js/src/jit/IonAnalysis.cpp
@@ +1294,5 @@
>  
>      if (test->getOperand(0) == input && direction == TRUE_BRANCH) {
>          *eliminated = true;
>          barrier->replaceAllUsesWith(barrier->input());
> +        if (input->isUnbox() && input->toUnbox()->mode() == MUnbox::TypeBarrier)

barrier->input() instead of input

@@ +1329,5 @@
>      }
>  
>      *eliminated = true;
>      barrier->replaceAllUsesWith(barrier->input());
> +    if (input->isUnbox() && input->toUnbox()->mode() == MUnbox::TypeBarrier)

barrier->input() instead of input

Comment 7

[Hannes Verschore [:h4writer]]

Backout due to orange:
http://hg.mozilla.org/integration/mozilla-inbound/rev/5da8f0c08bd0

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Looks like it was 64bit bustage.

Comment 9

[Hannes Verschore [:h4writer]]

I forgot to push the review comments and one of these changes was causing this orange. So I wouldn't have caught it on the try server push (even if I tried 64bit).

Re-pushed with review comments addressed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1bd3bb5a0ba

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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

Comment 11

[Boris Zbarsky [:bzbarsky]]

Either this bug or bug 914162 caused a 3% dromaeo-DOM regression on Mac....

Comment 12

[Boris Zbarsky [:bzbarsky]]

What about the regression?

Comment 13

[Hannes Verschore [:h4writer]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> What about the regression?

I'm still on planning to look into it. Being a dramaeo dom regression makes it harder to debug so takes more time. For the time being I'm having a nice improvement coming up for Typebarriers in bug 910960. I think this might remove/hide the current regression, until I find the real reason.