Note: There are a few cases of duplicates in user autocompletion which are being worked on.

add missing prop ic

RESOLVED FIXED in mozilla17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: luke, Assigned: luke)

Tracking

(Blocks: 1 bug)

unspecified
mozilla17
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

5 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.
Yeah, this should be very easy in either JIT. Can you attach the/a testcase you used to benchmark?
(Assignee)

Comment 2

5 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.
(Assignee)

Comment 3

5 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.
(Assignee)

Comment 4

5 years ago
Created attachment 646440 [details] [diff] [review]
WIP

Here's a basic WIP that works for my little test.
Assignee: general → luke
Status: NEW → ASSIGNED
Whiteboard: [js:t]
(Assignee)

Comment 5

5 years ago
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?
Attachment #646440 - Attachment is obsolete: true
Attachment #646653 - Flags: review?(bhackett1024)
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.
Attachment #646653 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 7

5 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'.
(Assignee)

Comment 8

5 years ago
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
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1667c960fc
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/3e1667c960fc
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.