Closed Bug 1124870 Opened 9 years ago Closed 9 years ago

Remove IC calls to LookupProperty

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: evilpie, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

There are two calls to LookupProperty, IsCacheableDOMProxyUnshadowedSetterCall in IonCaches.cpp and EffectlesslyLookupProperty in BaselineIC.cpp.

Can those be replaced, i.e. with LookupPropertyPure?
What is the difference between LookupProperty and LookupPropertyPure?
LookupPropertyPure, only works on native objects and doesn't work with most resolves hooks (except for function and string).
OK, so looking at IsCacheableDOMProxyUnshadowedSetterCall:

If the LookupProperty finds a prop on some holder, we then only do the fast thing with the holder if IsCacheableSetPropCallNative() or IsCacheableSetPropCallPropertyOp().  Both of these call IsCacheableProtoChainForIon(obj, holder), which verifies that everything starting at obj.__proto__ and up through and including "holder" is native.

So the only working on native objects is OK.

Not doing resolve hooks is less OK at first glance.  In particular, say the proto chain for a DOM proxy D looks like this:

   D -> R -> P

where R isNative() but has a resolve hook, which would shadow some property X on proto P.  We look up X on D, don't call the resolve hook, find it on P, and generate an IC which will jump directly to P, no?  So it seems like we need to either check for resolve hooks in IsCacheableProtoChainForIon or something?
I expect the baseline ICs are similar except with IsCacheableProtoChain instead of IsCacheableProtoChainForIon, but I haven't verified all the callsites.
Flags: needinfo?(jdemooij)
(In reply to Tom Schuster [:evilpie] from comment #0)
> There are two calls to LookupProperty,
> IsCacheableDOMProxyUnshadowedSetterCall in IonCaches.cpp and
> EffectlesslyLookupProperty in BaselineIC.cpp.

Bug 1135816 replaced the one in BaselineIC.cpp so that leaves IsCacheableDOMProxyUnshadowedSetterCall in IonCaches.cpp. I think we can just use LookupPropertyPure there too; will write a patch tomorrow.

(In reply to Not doing reviews right now from comment #3)
> We look up X on D, don't call the resolve hook, find it on P,
> and generate an IC which will jump directly to P, no?  So it seems like we
> need to either check for resolve hooks in IsCacheableProtoChainForIon or
> something?

LookupPropertyPure doesn't skip resolve hooks; it just gives up and returns false (unless our new mayResolve hook says its safe to ignore the resolve hook).
Attached patch Patch (obsolete) — Splinter Review
Use LookupPropertyPure in IsCacheableDOMProxyUnshadowedSetterCall.

I'm pretty happy all ICs use LookupPropertyPure now; so much better than the old/buggy situation where everybody called lookupProperty.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8599139 - Flags: review?(evilpies)
Comment on attachment 8599139 [details] [diff] [review]
Patch

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

::: js/src/jit/IonCaches.cpp
@@ +2656,1 @@
>          return false;

This looks incorrect, LookupPropertyPure returns false even when there is no error. You should remove the "isSetter" parameter from this function and have the return value indicate if it's a setter or not.
(In reply to Tom Schuster [:evilpie] from comment #7)
> You should remove the "isSetter" parameter from this function and
> have the return value indicate if it's a setter or not.

Oops, thanks. I thought IsCacheableDOMProxyUnshadowedSetterCall's return value already had that meaning and missed the isSetter outparam :(
Attached patch Patch v2Splinter Review
Attachment #8599139 - Attachment is obsolete: true
Attachment #8599139 - Flags: review?(evilpies)
Attachment #8599149 - Flags: review?(evilpies)
Comment on attachment 8599149 [details] [diff] [review]
Patch v2

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

::: js/src/jit/IonCaches.cpp
@@ +2657,3 @@
>  
>      if (!IsCacheableSetPropCallNative(checkObj, holder, shape) &&
>          !IsCacheableSetPropCallPropertyOp(checkObj, holder, shape) &&

I would have changed this to return IsCacheable ... || IsCacheabl, your call.
Attachment #8599149 - Flags: review?(evilpies) → review+
https://hg.mozilla.org/mozilla-central/rev/c23d24a503be
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.