Last Comment Bug 777630 - add missing prop ic
: add missing prop ic
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]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: gecko-games
  Show dependency treegraph
 
Reported: 2012-07-25 23:36 PDT by Luke Wagner [:luke]
Modified: 2012-07-31 19:18 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (10.25 KB, patch)
2012-07-26 18:28 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
patch (11.44 KB, patch)
2012-07-27 11:40 PDT, Luke Wagner [:luke]
bhackett1024: review+
Details | Diff | Splinter Review
better patch (8.79 KB, patch)
2012-07-31 09:09 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-07-25 23:36:36 PDT
If a property lookup misses (producing undefined), we disable the IC and stubcall every time.  I'm looking at a JS physics engine (closed atm, unfortunately) that is doing a ton of these, taking up to 20% of our time.  v8 seems to have an IC for this which makes them about 16x faster on a simple testcase.  Any chance we could get one too?  It seems like it would be pretty simple.
Comment 1 David Anderson [:dvander] 2012-07-26 00:31:50 PDT
Yeah, this should be very easy in either JIT. Can you attach the/a testcase you used to benchmark?
Comment 2 Luke Wagner [:luke] 2012-07-26 00:54:33 PDT
I was using:

  var o = {x:1, y:2};
  var s = 0;
  for (var i = 0; i < 1000000; ++i) {
    if (o.z)
      s++;
  }

for the simple test.  This test doesn't show it, but the IC would need to be polymorphic to be effective for the purposes of the physics engine.
Comment 3 Luke Wagner [:luke] 2012-07-26 01:17:56 PDT
Digging in a little more, it seems that the property sometimes exists (a small but non-trivial percent of the time).  Thus, to really help, it seems like this would need to be integrated into the general property IC.
Comment 4 Luke Wagner [:luke] 2012-07-26 18:28:38 PDT
Created attachment 646440 [details] [diff] [review]
WIP

Here's a basic WIP that works for my little test.
Comment 5 Luke Wagner [:luke] 2012-07-27 11:40:31 PDT
Created attachment 646653 [details] [diff] [review]
patch

This was eerily easy, so I'm worried I'm missing something.  I didn't touch TI; it seems like the type barriers would cover it; is that right?
Comment 6 Brian Hackett (:bhackett) 2012-07-27 11:59:03 PDT
Comment on attachment 646653 [details] [diff] [review]
patch

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

::: js/src/methodjit/PolyIC.cpp
@@ +599,5 @@
> +    GetPropLookup_Error = Lookup_Error,
> +    GetPropLookup_Uncacheable = Lookup_Uncacheable,
> +    GetPropLookup_Cacheable = Lookup_Cacheable,
> +    GetPropLookup_NoProperty
> +};

This enum seems to make things overly complicated.  Can you add Lookup_NoProperty to LookupStatus instead?  That requires going over the other places where the statuses are tested, but it looks like they are mostly correct already, testing for != Lookup_Cacheable rather than == Lookup_Uncacheable.

@@ +1278,5 @@
>  
> +        /*
> +         * A non-null 'shape' tells us where to find the property value in the
> +         * holder object. A null shape means that the above checks guard the
> +         * absence of the property, so the get-prop returns 'undefined'.

A comment on why it's ok to produce an undefined value without checking the type information would be good.  Since the stub call generating the cache will produce undefined, and the results of all getprop stub calls are monitored regardless, this will work.
Comment 7 Luke Wagner [:luke] 2012-07-27 12:25:54 PDT
(In reply to Brian Hackett (:bhackett) from comment #6)
> This enum seems to make things overly complicated.  Can you add
> Lookup_NoProperty to LookupStatus instead?

Adding a new case to an enum used in a wider context seems scary, but 'sure'.
Comment 8 Luke Wagner [:luke] 2012-07-31 09:09:06 PDT
Created attachment 647554 [details] [diff] [review]
better patch

Tests found two important cases where we need to disallow the missing-prop ic:
 - __noSuchMethod__
 - obj->getClass()->getProperty != JS_PropertyStub
non-standard SM-isms ftl
Comment 10 Ryan VanderMeulen [:RyanVM] 2012-07-31 19:18:53 PDT
https://hg.mozilla.org/mozilla-central/rev/3e1667c960fc

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