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.

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

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.

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 :(

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.