JM: fix arguments.length jsop_getprop fast path

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Right now arguments.length will always go through stubs::GetProp, even when !script->needsArgsObj() since the types->isMagicArguments test is inside the top->mightBeType(JSVAL_TYPE_OBJECT) branch, which is going to be false for optimized arguments.
(Assignee)

Comment 1

5 years ago
Created attachment 646326 [details] [diff] [review]
fix

This seems to give back 5% on earley-boyer.  This probably explains the regressions in bug 712714.
Assignee: general → luke
Status: NEW → ASSIGNED
Attachment #646326 - Flags: review?(bhackett1024)
Comment on attachment 646326 [details] [diff] [review]
fix

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

::: js/src/methodjit/Compiler.cpp
@@ +4785,5 @@
>          }
>          return true;
>      }
>  
> +    /* Handle lenth accesses of optimize 'arguments'. */

typos

@@ +4788,5 @@
>  
> +    /* Handle lenth accesses of optimize 'arguments'. */
> +    if (name == cx->runtime->atomState.lengthAtom &&
> +        cx->typeInferenceEnabled() &&
> +        analysis->poppedTypes(PC, 0)->isMagicArguments(cx) &&

the poppedTypes call can just be 'top', right?
Attachment #646326 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 3

5 years ago
I assumed there was some subtle difference between the type info in a FrameEntry and in poppedTypes since the code right below this uses both 'top' and 'types'.  Also, isMagicArguments is on the TypeSet.
(Assignee)

Comment 4

5 years ago
(Are they equivalent?  It isn't easy to see from the source.)
Oops, you're right, poppedTypes is better to use here as top is a FrameEntry rather than a TypeSet.
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/01a7a9d46117
Target Milestone: --- → mozilla17
Whiteboard: [js:t]
https://hg.mozilla.org/mozilla-central/rev/01a7a9d46117
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
What regressed this bug? Please set as blocked by this bug.

/be
(Assignee)

Updated

5 years ago
Blocks: 712714

Updated

5 years ago
Duplicate of this bug: 766075
You need to log in before you can comment on or make changes to this bug.