1239068 - Inline "PossiblyTypedArrayLength" intrinsic

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Morgan Phillips [:mrrrgn]]

As per https://bugzilla.mozilla.org/show_bug.cgi?id=1187232#c5

Comment 1

[Morgan Phillips [:mrrrgn]]

Attachment: inline.diff

Comment 2

[Jeff Walden [:Waldo]]

Comment on attachment 8708023 [details] [diff] [review]
inline.diff

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

::: js/src/jit/MCallOptimize.cpp
@@ +2269,5 @@
>  IonBuilder::InliningStatus
> +IonBuilder::inlinePossiblyWrappedTypedArrayLength(CallInfo& callInfo)
> +{
> +    // inlineTypedArrayLength will not inline wrapped objects since they do not
> +    // pass an IsTypedArrayObject test, which is what we want.

Huh, no kidding.  Mini-nit, tho: inside inlineTypedArrayLength is a stale comment about how the intrinsic is unreachable if the argument isn't a typed array, which (depending how you read it) is no longer true after this patch.  Could you flip these, so that inlineTypedArrayLength calls the Possibly variant, and move the stale comment (adjusted as necessary) into that method?  Just want to be clear that Possibly *does* have to consider that its argument might not be a typed array, where non-Possibly is just being slightly pessimal (and dealing with the possibility it's generating unreachable code).

Given I'm not sure what the comment wordings would look like for this, let's do one mini-pass on reviewing the updated patch.  This otherwise looks fine.

Comment 3

[Morgan Phillips [:mrrrgn]]

Attachment: inline.diff

Holy cow, good catch. I removed the last sentence, that suggested the code should never be reached.

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8708448 [details] [diff] [review]
inline.diff

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

::: js/src/jit/MCallOptimize.cpp
@@ +2269,5 @@
> +IonBuilder::inlinePossiblyWrappedTypedArrayLength(CallInfo& callInfo)
> +{
> +    // inlineTypedArrayLength will not inline wrapped objects since they do not
> +    // pass an IsTypedArrayObject test, which is what we want.
> +    return inlineTypedArrayLength(callInfo);

I meant to have these flipped -- name the *existing* method inlinePossiblyWrappedTypedArrayLength, then have inlineTypedArrayLength call iPWTAL.

Comment 5

[Morgan Phillips [:mrrrgn]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Comment on attachment 8708448 [details] [diff] [review]
> inline.diff
> 
> Review of attachment 8708448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/MCallOptimize.cpp
> @@ +2269,5 @@
> > +IonBuilder::inlinePossiblyWrappedTypedArrayLength(CallInfo& callInfo)
> > +{
> > +    // inlineTypedArrayLength will not inline wrapped objects since they do not
> > +    // pass an IsTypedArrayObject test, which is what we want.
> > +    return inlineTypedArrayLength(callInfo);
> 
> I meant to have these flipped -- name the *existing* method
> inlinePossiblyWrappedTypedArrayLength, then have inlineTypedArrayLength call
> iPWTAL.

Me IRL: http://img13.deviantart.net/20eb/i/2012/153/e/a/whoosh_by_medli20-d520mia.gif

Comment 6

[Morgan Phillips [:mrrrgn]]

Also, thanks again for the patience, and the reviews.

Comment 7

[Morgan Phillips [:mrrrgn]]

Attachment: inline.diff

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8709630 [details] [diff] [review]
inline.diff

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

::: js/src/jit/MCallOptimize.cpp
@@ +2253,5 @@
>          return InliningStatus_NotInlined;
>      if (getInlineReturnType() != MIRType_Int32)
>          return InliningStatus_NotInlined;
>  
>      // Note that the argument we see here is not necessarily a typed array.

This comment could probably go away now that it's obvious the method can be given non-typed arrays.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/338294fa149d

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/338294fa149d