The testcase in bug 962256 basically does this: document.id = function() { /* stuff */ }; and then later does a bunch of document.id() calls. For us, the get of document.id ends up taking the slow path through the proxy get IC, because all we know is that the property is shadowed. But in practice, we have different kinds of shadowing. There's shadowing due to the property living on the unforgeable holder. There's shadowing due to the property living on the expando object. And there's shadowing due to the property being found by the named getter magic. This case is the second of those. In our current impl we seem to look at the expando object before the named getter. Per spec, I think it should be after, actually... Anyway, if we said in our shadowing return value what sort of shadowing is going on, it seems like the JIT could in fact optimize the "shadows due to expando" case much better than we do right now; basically just get it off the expando object if our guards pass.

Jan, this is the sort of thing that your IC rearch might make much simpler to do...

I'm not going to have time to work on this stuff in the foreseeable future, and chances are those WIP patches are not the right way forward anyway; instead CacheIR is the way to go.

Yeah these things should be much easier now. NI myself to see if I can get something working, but if it's complicated I will work on this after doing other IC work.

Here's the CacheIR version of your Baseline patch. It improves the testcase attached here from 68 ms to 6 ms or so on OS X 64-bit. bz, a question about ExpandoAndGeneration (see the XXX in the patch). When we have an ExpandoAndGeneration at IC compile time, does the shape guard ensure we always have an ExpandoAndGeneration at runtime as well? If not, we have to add an extra guard instead of the LoadDOMExpandoFromExpandoAndGeneration codegen in this patch.

I just remembered the expando getter testcase in bug 1128157, so this patch supports expando getters as well. For the jsperf test (http://jsperf.com/js-getter-as-perfomance-boost) from that bug I get: Normal node list: 380,326 ops/second (no change) "Boosted" node list: 24,046 ops/second -> 701,549 ops/second So the boosted node list is now 30 times faster and much faster than the normal length getter.

Sorry for the bugspam. This fixes a minor issue where Baseline would fall back to the generic shadowing-proxy code when we had a scripted getter without JIT code. Now we check isTemporarilyUnoptimizable_ and wait until the getter has JIT code instead of attaching the slower stub that calls Proxy::get. When I disable Ion, I get: Normal node list: 194,763 ops/second "Boosted" node list: 347,839 ops/second So this is very decent for Baseline: Ion is only about twice as fast. Ion could be even faster for the "boosted" case if it inlined the getter, bug 1324561 should let us do that I think.

This renames some of the CacheIR instructions we use for DOM proxies to be clearer, and also adds some documentation. No change in behavior.

Regarding my question in comment 6, I decided to just reuse LoadDOMExpandoValueGuardGeneration. That means we don't need any new CacheIR instructions to implement this, and it's also safer. It does add extra guards in the ExpandoAndGeneration case, but I don't see much of a perf difference.

Just noticed GuardDOMExpandoShapeIfPresent is missing an is-object check. Regression from bug 1319437, it was in the old Baseline and Ion code.

Comment on attachment 8823075 [details] [diff] [review] Part 3 - Fix GuardDOMExpandoShapeIfPresent Review of attachment 8823075 [details] [diff] [review]: ----------------------------------------------------------------- This guarding against the PrivateValue for ExpandoAndGeneration, I assume?

Comment on attachment 8823074 [details] [diff] [review] Part 2 - Optimize expando properties Review of attachment 8823074 [details] [diff] [review]: ----------------------------------------------------------------- How well is this covered by tests on the DOM side? Especially scripted and native getters on the expando. ::: js/src/jit/CacheIR.cpp @@ +580,5 @@ > + CanAttachNativeGetProp(cx_, expandoObj, id, &holder, &propShape, pc_, engine_, > + canAttachGetter_, isTemporarilyUnoptimizable_); > + if (canCache != CanAttachReadSlot && canCache != CanAttachCallGetter) > + return false; > + if (holder != expandoObj) This sort of obfuscates the condition. holder is either nullptr or expandoObject, because expandos don't have a prototype. I would suggest changing this to if (!holder) return false; MOZ_ASSERT(holder == expandoObj); @@ +606,5 @@ > + // Call the getter. Note that we pass objId, the DOM proxy, as |this| > + // and not the expando object. > + MOZ_ASSERT(canCache == CanAttachCallGetter); > + EmitCallGetterResultNoGuards(writer, expandoObj, expandoObj, propShape, objId); > + } So much shared code \o/

(In reply to Tom Schuster [:evilpie] from comment #15) > How well is this covered by tests on the DOM side? Especially scripted and > native getters on the expando. I'll forward this to bz. If we need new tests for this, where should I add them?

Sorry for the lag here; I was on PTO. > When we have an ExpandoAndGeneration at IC compile time, does the shape guard > ensure we always have an ExpandoAndGeneration at runtime as well? Whether we have an ExpandoAndGeneration is entirely determined by our JSClass. So if the shape guard ensures a particular JSClass, then it ensures this. I believe it does that, right? > How well is this covered by tests on the DOM side? Especially scripted and native getters on the expando. Probably not very well. We should add some tests. They can go in dom/bindings/test as mochitests.

And we should test both sorts of objects (simple expando and expando-and-generation), I guess. You can get a thing with a generation by getting document.documentElement.dataset. You can get a thing with a simple expando by getting document.getElementsByTagName("*").

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #17) > Whether we have an ExpandoAndGeneration is entirely determined by our > JSClass. So if the shape guard ensures a particular JSClass, then it > ensures this. I believe it does that, right? Yes, it does. With my current patches, we do guard we have an ExpandoAndGeneration (we did/do the same for the unshadowed guards). I think I'd prefer keeping these guards for now, to not rely too much on the Gecko setup. It doesn't affect perf much - this is a big win anyway. (I suppose a follow-up could add debug-only guards that assert on failure...) > Probably not very well. We should add some tests. They can go in > dom/bindings/test as mochitests. Thanks, I'll write them today.

I hit bug 1328835 for the dataset tests, but for NodeList they all pass. I also confirmed these tests fail when commenting out some of the guards we emit.

> I hit bug 1328835 for the dataset tests Sorry for the bad advice there. Defining accessor properties on dataset is actually impossible per spec... Use an HTMLFormElement instead. See bug 1328835 comment 2.

Thanks, everything works fine with HTMLFormElement. I double checked we attach a stub for all of these cases and I also verified HTMLFormElement has an ExpandoAndGeneration*.

Comment on attachment 8823072 [details] [diff] [review] Part 1 - Rename CacheIR instructions, add comment >+// * GuardDOMExpandoShapeIfPresent: takes an expando Value as input, then guards This is probably fine, but the key way this is used is to guard that an expando does not shadow something, which just happens to be true if there is no expando. So it's more like GuardDOMExpandoMissingOrHasMatchingShape, maybe... Either way, though. r=me

Comment on attachment 8823074 [details] [diff] [review] Part 2 - Optimize expando properties >+GetPropIRGenerator::tryAttachDOMProxyExpando(HandleObject obj, ObjOperandId objId, HandleId id) >+ MOZ_ASSERT(!expandoVal.isUndefined()); How about passing "How did a missing expando manage to shadow things?" as the second arg here? >+ expandoAndGeneration = (ExpandoAndGeneration*)expandoVal.toPrivate(); Not static_cast? Similar elsewhere. >+ expandoValId = writer.loadDOMExpandoValueGuardGeneration(objId, expandoAndGeneration); Hmm. So I think this could actually bite us in interesting ways in a non-microbenchmark situation. This adds two unnecessary guards: 1) A guard on the pointer identity of the ExpandoAndGeneration*. 2) A guard on the generation. If we have two different objects (e.g. two different forms) come through here, the first guard will cause us to miss the IC, since it's basically a guard on the object identity of "obj". If we have the same object come through twice with a DOM mutation in between, the second guard will cause us to miss the IC. Note that we are already assuming that the shape check on "obj" ensures that we have an ExpandoAndGeneration if it passes; loadDOMExpandoValueGuardGeneration assumes just that. So I don't think doing an explicit no-guards load of the expando value from the ExpandoAndGeneration would be any less safe than what we have now. So it really might be a good idea to have a CacheIR instruction for getting the expando from a known expando-and-generation proxy, to avoid those pitfalls. The rest of this looks beautiful. r=me, but please to think about the guard situation.

Comment on attachment 8823075 [details] [diff] [review] Part 3 - Fix GuardDOMExpandoShapeIfPresent CacheIRWriter::GuardDOMExpandoShapeIfPresent is CheckDOMProxyExpandoDoesNotShadow which only calls it when expandoVal.isObject(). Since at that point we've already shape-checked the obj argument to CheckDOMProxyExpandoDoesNotShadow, we know "obj" is a DOM proxy that has either undefined or an object in the expando slot. So this guard that's being added is completely redundant, as far as I can tell. In a debug build, it might be nice to assert here or something, I guess... But I see no reason to add this guard.

> CacheIRWriter::GuardDOMExpandoShapeIfPresent is CheckDOMProxyExpandoDoesNotShadow I meant "The only caller of CacheIRWriter::GuardDOMExpandoShapeIfPresent ... [etc]"

Comment on attachment 8824103 [details] [diff] [review] Part 4 - Tests r=me; this looks great. Thank you!

This version renames GuardDOMExpandoObject to GuardDOMExpandoMissingOrGuardShape intead of GuardDOMExpandoShapeIfPresent.

This adds a LoadDOMExpandoValueIgnoreGeneration instruction that does not have the guards we have for LoadDOMExpandoValueGuardGeneration. It does check in debug builds that the value is a PrivateValue (well, a double, as we can't distinguish these).

In debug builds assert the Value is an object.

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #0) > In our current impl we seem to look at the expando object before the named > getter. Per spec, I think it should be after, actually... Do you still think we should look at the named getter first? If we need to do that, we do have to the guard on the generation, right?

> Do you still think we should look at the named getter first? No. The spec got changed to align with our impl here, because what the spec used to say was an ES invariant violation. See https://github.com/heycam/webidl/issues/152 and bug 1297411. So at this point, the expando object always comes first, for both types of DOM proxies. Will do the reviews in ~2 hours.

Comment on attachment 8824399 [details] [diff] [review] Part 2 - Optimize expando properties Review of attachment 8824399 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, I leave it to bz to make sure that ignoring the generation is okay. Honestly I am not even sure what that is good for.

Comment on attachment 8824398 [details] [diff] [review] Part 1 - Rename CacheIR instructions, add comment r=me

Comment on attachment 8824399 [details] [diff] [review] Part 2 - Optimize expando properties r=me. Thank you!

Comment on attachment 8824400 [details] [diff] [review] Part 3 - Add assert to GuardDOMExpandoMissingOrGuardShape r=me

Sorry for the lag here. :( > Honestly I am not even sure what that is good for The generation is for when the set of own properties produced by the named getter changes. This can affect whether we shadow properties up our proto chain, so anything that finds a prop on the proto chain needs to guard on the generation. In this case, though, we're working with the expando object, which is looked at before anything else, and hence it doesn't matter what happens to the named getter's property set: if the prop is on the expando object, then we should get it from there.

Pushed by jandemooij@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1f5fe007fc9c part 1 - Rename some CacheIR instructions related to DOM proxies. r=bz,evilpie https://hg.mozilla.org/integration/mozilla-inbound/rev/744e3371701b part 2 - Add inline caches for getting DOM expando properties. r=bz,evilpie https://hg.mozilla.org/integration/mozilla-inbound/rev/f004cbe6f011 part 3 - Add an is-object debug assert to GuardDOMExpandoMissingOrGuardShape. r=bz https://hg.mozilla.org/integration/mozilla-inbound/rev/f9743055774c part 4 - Add tests. r=bz

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #37) > Sorry for the lag here. :( No worries, after almost 3 years a few days more or less really doesn't matter ;)

I was wondering how much this matters for normal websites, so I added some logging to count the number of times these expando stubs *succeed* (all guards pass and we optimized the lookup). Just starting Firefox and closing it results in 5 hits. On pretty much all non-static websites (Gmail, GDocs, Amazon, GitHub) I see at least a few hundred hits, pretty nice. The more interesting cases: * Loading Treeherder: > 700 hits * Loading facebook.com/BarackObama and scrolling down: > 1500 hits * Loading cnn.com: > 2000 hits * Loading twitter.com/firefox and scrolling down a little: > 2500 hits So this should shave off CPU cycles for everyone, and I'm sure there are even more excessive cases.

https://hg.mozilla.org/mozilla-central/rev/1f5fe007fc9c https://hg.mozilla.org/mozilla-central/rev/744e3371701b https://hg.mozilla.org/mozilla-central/rev/f004cbe6f011 https://hg.mozilla.org/mozilla-central/rev/f9743055774c

Is it possible to uplift this patch to Aurora (52), before it's too late for older Windows?

(In reply to Krzysztof from comment #42) > Is it possible to uplift this patch to Aurora (52), before it's too late for > older Windows? Unfortunately not. The patches here are pretty small, but for this to work on 52 we would have to backport a lot of other bugs, like bug 1322093.

> so I added some logging to count the number of times these expando stubs *succeed* Nice! Thank you again for doing this!