Luke Wagner [:luke] Assignee
	Description • 12 years ago

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.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 1 • 12 years ago

Yeah, this should be very easy in either JIT. Can you attach the/a testcase you used to benchmark?

	Luke Wagner [:luke] Assignee
	Comment 2 • 12 years ago

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.

	Luke Wagner [:luke] Assignee
	Comment 3 • 12 years ago

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.

	Luke Wagner [:luke] Assignee
	Comment 4 • 12 years ago

Here's a basic WIP that works for my little test.

	Luke Wagner [:luke] Assignee
	Comment 5 • 12 years ago

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?

	Brian Hackett [Laid off!]
	Comment 6 • 12 years ago

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.

	Luke Wagner [:luke] Assignee
	Comment 7 • 12 years ago

(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'.

	Luke Wagner [:luke] Assignee
	Comment 8 • 12 years ago

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

	Luke Wagner [:luke] Assignee
	Comment 9 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1667c960fc

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/3e1667c960fc