Last Comment Bug 777630 - add missing prop ic
: add missing prop ic
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla17
Assigned To: Luke Wagner [:luke]
: Jason Orendorff [:jorendorff]
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:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

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 User image 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 User image 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 User image 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)

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 User image 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 User image Luke Wagner [:luke] 2012-07-26 18:28:38 PDT
Created attachment 646440 [details] [diff] [review]

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

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 User image Brian Hackett (:bhackett) 2012-07-27 11:59:03 PDT
Comment on attachment 646653 [details] [diff] [review]

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 User image 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 User image 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 User image Ryan VanderMeulen [:RyanVM] 2012-07-31 19:18:53 PDT

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