Like bug 1089050 but for getters. This part might get a bit complicated.

I'll see if I can find time for this, but we have a bunch of IsCacheableGetPropCall callers, looks like, and I assume I need to refactor all of them. And I'm not sure about ordering wrt typemonitor stubs and the like....

Johannes, are you interested in taking a look at this one?

Definitely, sounds interesting. Not sure, if I'm able to get to it next week, but the week after that, I can definitely look into this. Feel free to take it, if it's urgent.

This isn't urgent; happy to leave it for you. ;)

Passes tests and improves microbenchmark by ~50%.

I just want to ensure that we can do this pre-emitting with respect to attaching fallback stubs and checking stub invalidation. As long as that is true, then these patches should look good. ni? djvj

ni? jandem for another possible pair of eyes.

Comment on attachment 8530933 [details] [diff] [review] Part 1: GETPROP Review of attachment 8530933 [details] [diff] [review]: ----------------------------------------------------------------- Refactor looks good. I'm a little concerned about the new handling of MAX_OPTIMIZED_STUBS. See below. Discussion welcome. ::: js/src/jit/BaselineIC.cpp @@ +7008,5 @@ > + return false; > + } > + > + // After the Genericstub was added, we should never reach the Fallbackstub again. > + MOZ_ASSERT(!stub->hasStub(ICStub::GetProp_Generic)); This assert seems out to place, see below. @@ +7026,5 @@ > + > + if (attached) > + return true; > + > + if (stub->numOptimizedStubs() >= ICGetProp_Fallback::MAX_OPTIMIZED_STUBS) { So now we can have unlimited numbers of getter stubs, right? ``` var i; var o = {}; for (i = 0; i < BIG_NUMBER; i++) { Object.defineProperty(o, "x", { get: (() => 4) }); o.x; } ``` I think the right thing to do here is to try and reset the stubs right at the beginning with the assert above it. If we did that, don't try to emit the getter stub, and just do the ComputeGetPropResult call through adding the monitor stub, and then return.

(In reply to Eric Faust [:efaust] from comment #11) > ni? jandem for another possible pair of eyes. We discussed the stub invalidation changes on IRC a while ago, so clearing NI.

Comment on attachment 8542298 [details] [diff] [review] Part 1: GETPROP Review of attachment 8542298 [details] [diff] [review]: ----------------------------------------------------------------- Good. I'm glad that worked out to be as easy as it seemed in the last review. Thanks for doing this. r=me

Comment on attachment 8542301 [details] [diff] [review] PART 2: GETELEM Review of attachment 8542301 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: js/src/jit/BaselineIC.cpp @@ +3793,5 @@ > > > +static bool TryAttachNativeGetValueElemStub(JSContext *cx, HandleScript script, jsbytecode *pc, > + ICGetElem_Fallback *stub, HandleObject obj, > + HandleValue key) I understand why this doesn't take an |attached| outparam (it will be ignored in the current codeflow), but it's a little weird to see the difference.

Comment on attachment 8542302 [details] [diff] [review] Part 3: GETGNAME Review of attachment 8542302 [details] [diff] [review]: ----------------------------------------------------------------- Yep. ::: js/src/jit/BaselineIC.cpp @@ +6036,5 @@ > + MOZ_ASSERT(global->is<GlobalObject>()); > + > + RootedId id(cx, NameToId(name)); > + > + // The property must be found, and it must be found as a normal data property. I very much doubt that ;)

Comment on attachment 8530939 [details] [diff] [review] Part 4: Add assert in UpdateExistingGetPropCallStubs Review of attachment 8530939 [details] [diff] [review]: ----------------------------------------------------------------- :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/e68045bf8272 https://hg.mozilla.org/integration/mozilla-inbound/rev/97b21e438a35 https://hg.mozilla.org/integration/mozilla-inbound/rev/f77994182908 https://hg.mozilla.org/integration/mozilla-inbound/rev/3693f117ad50

https://hg.mozilla.org/mozilla-central/rev/e68045bf8272 https://hg.mozilla.org/mozilla-central/rev/97b21e438a35 https://hg.mozilla.org/mozilla-central/rev/f77994182908 https://hg.mozilla.org/mozilla-central/rev/3693f117ad50

Clearing needinfo.