IonMonkey: Typebarrier improvements

RESOLVED FIXED in mozilla26

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: h4writer, Assigned: h4writer)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

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...
Assignee: general → hv1989
Attachment #795980 - Flags: review?(dvander)
Attachment #795980 - Flags: review?(dvander) → review+
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/
Attachment #799128 - Flags: review?(jdemooij)
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.
Attachment #799128 - Flags: review?(jdemooij) → review+
Posted patch bug909717-part3 (obsolete) — Splinter Review
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.
Attachment #800207 - Flags: review?(jdemooij)
forget to qref
Attachment #800207 - Attachment is obsolete: true
Attachment #800207 - Flags: review?(jdemooij)
Attachment #800209 - Flags: review?(jdemooij)
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
Attachment #800209 - Flags: review?(jdemooij)
Looks like it was 64bit bustage.
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
https://hg.mozilla.org/mozilla-central/rev/a1bd3bb5a0ba
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Either this bug or bug 914162 caused a 3% dromaeo-DOM regression on Mac....
Flags: needinfo?(hv1989)
Depends on: 916752
Depends on: 915171
Flags: needinfo?(hv1989)
What about the regression?
Flags: needinfo?(hv1989)
(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.
Flags: needinfo?(hv1989)
Blocks: 983709
You need to log in before you can comment on or make changes to this bug.