Last Comment Bug 777643 - JM: fix arguments.length jsop_getprop fast path
: JM: fix arguments.length jsop_getprop fast path
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
:
Mentors:
: 766075 (view as bug list)
Depends on:
Blocks: 712714
  Show dependency treegraph
 
Reported: 2012-07-26 00:28 PDT by Luke Wagner [:luke]
Modified: 2012-11-02 05:16 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix (2.78 KB, patch)
2012-07-26 14:06 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Review

Description Luke Wagner [:luke] 2012-07-26 00:28:17 PDT
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.
Comment 1 Luke Wagner [:luke] 2012-07-26 14:06:32 PDT
Created attachment 646326 [details] [diff] [review]
fix

This seems to give back 5% on earley-boyer.  This probably explains the regressions in bug 712714.
Comment 2 Brian Hackett (:bhackett) 2012-07-26 14:13:04 PDT
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?
Comment 3 Luke Wagner [:luke] 2012-07-26 14:17:29 PDT
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.
Comment 4 Luke Wagner [:luke] 2012-07-26 15:16:44 PDT
(Are they equivalent?  It isn't easy to see from the source.)
Comment 5 Brian Hackett (:bhackett) 2012-07-26 15:43:58 PDT
Oops, you're right, poppedTypes is better to use here as top is a FrameEntry rather than a TypeSet.
Comment 7 :Ehsan Akhgari (out sick) 2012-07-27 08:59:07 PDT
https://hg.mozilla.org/mozilla-central/rev/01a7a9d46117
Comment 8 Brendan Eich [:brendan] 2012-08-21 15:43:30 PDT
What regressed this bug? Please set as blocked by this bug.

/be
Comment 9 Jan de Mooij [:jandem] 2012-11-02 05:16:08 PDT
*** Bug 766075 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.