Closed Bug 937772 Opened 6 years ago Closed 6 years ago

Should emit an infallible unbox in pushTypeBarrier when we can

Categories

(Core :: JavaScript Engine: JIT, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 2 obsolete files)

Right now pushTypeBarrier has a "needsBarrier" argument that basically conflates two things: whether we need a type barrier and whether we can unbox infallibly.

In most cases those are in fact the same thing, but an exception is when we know we have an object.  In that case we still need the barrier, but we can do an infallible unbox.

So for example on this testcase:


  var d = document.createElement("canvas").getContext("2d").createImageData(1, 1);
  var count = 10000000;
  var z;
  for (var i = 0; i < count; ++i) {
    z = d.data;
  }

we end up with this code after the GetDOMProperty finishes:

  [Unbox:Object]
    movq       %rcx, %r11
    shrq       $47, %r11
    cmpl       $0x1fff7, %r11d
    jne        ((2096))
    movabsq    $0x7fffffffffff, %rax
    andq       %rcx, %rax
  [TypeBarrierO]
    movabsq    $0x11d541a60, %r11
    cmpq       %r11, %rax
    jne        ((2128))

whereas with the patch coming up we get this:

  [Unbox:Object]
    movabsq    $0x7fffffffffff, %rax
    andq       %rcx, %rax
  [TypeBarrierO]
    movabsq    $0x10b14c880, %r11
    cmpq       %r11, %rax
    jne        ((2108))
Oh, I'm not sure whether the comment before pushTypeBarrier needs updating due to this change or not...
Comment on attachment 830986 [details] [diff] [review]
Make better use of our out-of-band type information for unboxing object-valued return values of DOM getters and methods.

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

r=me with pushTypeBarrier invocation fixed in makeCall()

::: js/src/jit/IonBuilder.cpp
@@ +5268,3 @@
>      }
>  
>      return pushTypeBarrier(call, types, barrier);

uh, don't we need to pass the hint here?
Attachment #830986 - Flags: review?(efaustbmo) → review+
> uh, don't we need to pass the hint here?

Er... yes.  I should find a case that would test this and verify that it actually works once I do that!
Element.getElementsByTagName is a good testcase, and I verified that it's broken with this patch as-is and fixed when I pass the new arg there.  Optional args suck.  :(
So if I understand correctly
1) we now have jitInfo()->returnType, that contains the type a target will definitely return.
2) we have "types", that contain the observed types.

In other words (1) can be infallible, since we are sure that this is the right value and (2) needs a barrier.

I think the patch here is doing it wrongly.

The current way is:
> MGetDOMProperty *get = MGetDOMProperty::New(jitinfo, obj, guard);
> current->add(get);
> current->push(get);
>
> if (get->isEffectful() && !resumeAfter(get))
>     return false;
> bool barrier = DOMCallNeedsBarrier(jitinfo, types);
> if (!pushTypeBarrier(get, types, barrier))
>     return false;

And should get transformed to:
>     MGetDOMProperty *get = MGetDOMProperty::New(jitinfo, obj, guard);
>     current->add(get);
>     current->push(get);
>
>     if (get->isEffectful() && !resumeAfter(get))
>         return false;
>
>     // Unbox output to type this call will definitely return
> (a) if (jitInfo.returnType != JSVAL_TYPE_UNKNOWN) {
>         MInstruction *unbox = MUnbox::New(get, MIRTypeFromValueType(jitInfo.returnType), MUnbox::Infallible);
>         current->push(unbox);
>     }
>
> (b) bool barrier = DOMCallNeedsBarrier(jitinfo, types);
>
>     // Add typebarriers to keep track of observed values
> (c) if (!pushTypeBarrier(current->peek(-1), types, barrier))
>         return false;

So (a) doing part (1) and (c) doing part (2) of the story.

I also have a small note for (b). This can get safely removed and always return "true" to pushTypeBarrier. TypeBarrier is/should now be smart enough to do that on it's own, without needing those hints. (If not, this is a bug that should get fixed). Note 2 for (b). The first comment in DOMCallNeedsBarrier contradicts the return value. It says there is no need to have a barrier, but happily returns true;

Another note for (a). This can also use pushTypeBarrier(get, CONVERT_JSVAL_TO_TYPESET, false). But that needs to allocate a TemporaryTypeSet.
Attachment #830986 - Attachment is obsolete: true
Comment on attachment 832273 [details] [diff] [review]
Make better use of our out-of-band type information for unboxing object-valued return values of DOM getters and methods.

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

Good work, thanks for doing this. I'm not putting r+, since I don't understand the "Note".
Changing pushTypeBarrier to take MDefinition instead of MInstruction is not really required for r+. It is a bonus ;).

::: js/src/jit/IonBuilder.cpp
@@ +5244,5 @@
>  
>      types::TemporaryTypeSet *types = bytecodeTypes(pc);
>  
>      bool barrier = true;
> +    MInstruction* replace = call;

Style nit: MInstruction *replace.
The asterisk should be nexto the name. This isn't the only place, there are other places where you did this.

@@ +5250,5 @@
>          JSFunction* target = call->getSingleTarget();
>          JS_ASSERT(target && target->isNative() && target->jitInfo());
> +        const JSJitInfo *jitinfo = target->jitInfo();
> +        barrier = DOMCallNeedsBarrier(jitinfo, types);
> +        replace = EnsureDefiniteType(call, jitinfo->returnType)->toInstruction();

The toInstruction() is only needed for pushTypeBarrier right? I just checked and I think you can transform pushTypeBarrier to take an MDefinition. This would remove this problem and I see no reason why pushTypeBarrier should take a MInstruction.

@@ +6132,5 @@
>      current->push(barrier);
>      return true;
>  }
>  
> +MDefinition*

Style nit: space between MDefinition and *

@@ +6133,5 @@
>      return true;
>  }
>  
> +MDefinition*
> +IonBuilder::EnsureDefiniteType(MDefinition* def, JSValueType definiteType)

Style nit: MDefinition *def

@@ +6135,5 @@
>  
> +MDefinition*
> +IonBuilder::EnsureDefiniteType(MDefinition* def, JSValueType definiteType)
> +{
> +    MInstruction* replace;

Style nit: MInstruction *replace

@@ +8317,5 @@
>  
>          if (get->isEffectful() && !resumeAfter(get))
>              return false;
>          bool barrier = DOMCallNeedsBarrier(jitinfo, types);
> +        MInstruction* replace = EnsureDefiniteType(get, jitinfo->returnType)->toInstruction();

Style nit: *

::: js/src/jit/IonBuilder.h
@@ +338,5 @@
> +    // returns def.  Otherwise, returns an MInstruction that has that definite
> +    // type, infallibly unboxing ins as needed.  The new instruction will be
> +    // added to |current| in this case.
> +    // Note that if def is an MInstruction, then this will always return an
> +    // MInstruction.

I don't really understand this note?
Attachment #832273 - Flags: review?(hv1989) → feedback+
> The toInstruction() is only needed for pushTypeBarrier right?

Yes.  I'll see if I can make that take an MDefinition.

> I don't really understand this note?

That note is what makes the unconditional toInstruction() calls safe.  If those go away, I'll get rid of the note too.
Attachment #832304 - Flags: review?(hv1989)
Attachment #832273 - Attachment is obsolete: true
Comment on attachment 832304 [details] [diff] [review]
Updated to comments

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

Awesome!

::: js/src/jit/IonBuilder.h
@@ +338,5 @@
> +    // returns def.  Otherwise, returns an MInstruction that has that definite
> +    // type, infallibly unboxing ins as needed.  The new instruction will be
> +    // added to |current| in this case.
> +    // Note that if def is an MInstruction, then this will always return an
> +    // MInstruction.

Don't forget to remove the note ;)
Attachment #832304 - Flags: review?(hv1989) → review+
Did that.  Also added the missing break statements that asserts caught.  ;)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e54e176909
Flags: in-testsuite?
Whiteboard: [need review]
Target Milestone: --- → mozilla28
https://hg.mozilla.org/mozilla-central/rev/b1e54e176909
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 939835
You need to log in before you can comment on or make changes to this bug.