Just filing this so we have a bug tracking this. The current plan is to effectively give documents a shape, update the shape when the name set in the document changes, and expose it to the jit via possibly some virtual proxy handler methods that return an offset into the document object. Then the JIT can guard on that shape and if that succeeds compile a direct get on the proto.

I'm going to need some help from JS folks for this. In the DOM we have proxies for which properties from the prototype shadow properties on the object. The property get IC has support for that by detecting these objects and skipping to the prototype directly (see bug 769911). But now we're adding proxies for which this special shadowing doesn't happen (in particular for HTMLDocument). We'd still like property gets for properties from the prototype to be fast by skipping directly to the prototype. We can easily add a way to detect that the set of own properties of the proxy has changed (eg by incrementing a counter when a property is added or removed) so we'd like to change the property get IC to use that information in some way so it can skip to the prototype but bail out if the property on the prototype might be shadowed by an own property on the proxy. It's fairly easy to add a guard to the IC for property gets to check object identity and a counter for our document objects. But I worry that we'll hit MAX_PIC_STUBS pretty quickly for these objects (multiple properties appearing and disappearing on the proxy would cause this). So one solution I thought about would be this: if the guard for the counter fails we bail out and try to get the property again. If the property should still be gotten from the prototype we'd update the counter check in the stub to compare against the new value of the counter, instead of creating a new stub. I have a lot of the machinery for this working, but I've hit a problem: once the guard for the counter value fails we bail out and at that point I don't have a way to know which stub should be updated. AFAICT there's no state attached to the stubs that I could use to determine which one applies to the current object. So I have two questions for the JS peers: 1) Does this approach make any sense to you? 2) If so, how do we figure out which stub we need to update? Setting needinfo to :bhackett since he did the original DOM proxy changes to the property get IC. Feel free to kick the ball to someone more appropriate :-).

CC'ing Kannan since he did some these ICs for IonMonkey. A few questions: (1) Are you seeing the MAX_PIC_STUBS limit get reached? (2) Do you need to modify a generated stub, or can you slightly generalize the stub code more? (3) Are you doing this against JM? (JM is scheduled for removal in Fx24 or 25, so it might be better to focus on Ion if so)

Are the only objects of interest here documents? It seems like in that case then each access site will typically see only one object, so this approach should work well. As for figuring out stubs, the only JIT you should worry about optimizing for is Ion. JM will be at EOL soon and Baseline hasn't landed yet (though, make sure that JM's ListBase cache doesn't do the wrong thing on these documents). For Ion, there isn't really a way to figure out the stub to modify, Ion doesn't keep track of the stubs it generates for a cache or where they are. Two options I see: a) Just reset() the IonCache before generating one of these new stubs, which will unlink all stubs in the IC and keep you from filling up the stub chain with out of date ICs. This should work fine unless the counter changes very frequently. b) Have the stub dynamically load the counter to use from some fixed address; the IonCache can then just update the counter stored in the address to change the stub's behavior. The main problem with this is I don't see a great place to store the counter for the stub to load from; the best option is as a new field of GetPropertyIC, but that will bloat the structure's size for a rare case. I doubt that would be a problem, but option (a) is still easier and my preference.

Peter: How often do you expect these pseudo-shapes to change? If you're expecting them to change infrequently, then bhackett's choice (a) is probably the way to go. The appropriate stubs will get re-generated quickly, and it's straightforward. If you're expecting them to change more frequently, then (b) from the post above makes more sense. Frequently or infrequently is hard to define, so let's say this: you visit a webpage, and hang around for a minute or so, and the page is doing the javascripts for that duration. Do we expect these shapes to change: 1) a few times? 2) a few dozen times? 3) a few hundred or more times? Is this something content scripts can influence? I.e. somebody can write JS code that causes these shapes to change arbitrarily fast? If so, what kind of code would people have to write to induce that behaviour? > Originally posted by bhackett: > b) Have the stub dynamically load the counter to use from some fixed > address; the IonCache can then just update the counter stored in the address > to change the stub's behavior. The main problem with this is I don't see a > great place to store the counter for the stub to load from; the best option > is as a new field of GetPropertyIC, but that will bloat the structure's size > for a rare case. I doubt that would be a problem, but option (a) is still > easier and my preference. We could code also the stub to have a patchable constant which the stub repatches itself whenever the shape check fails. But yeah, (a) is better if we can get away with it.

(In reply to Brian Hackett (:bhackett) from comment #3) > Are the only objects of interest here documents? It seems like in that case > then each access site will typically see only one object, so this approach > should work well. Not the only one; form elements get the same treatment.

This is needed for document and forms. There can be multiple documents visible to a script, with XHR result documents (though I'm hoping we can make those not be ListBase, maybe; need to get the the spec changed). The changes in the pseudo-shape can happen pretty often: any time an element with an id or name is added to or removed from the document, afaict (or any time inputs are added to or removed from a form). We may be able to restrict it to some particular element types, maybe. Not sure yet. If we can, that ought to make the changes rare for documents and maybe rare for forms, so maybe that's good enough... Web page script can obviously trigger these changes whenever it wants by inserting/removing the right sort of elements.

Based on that, I'd say we can expect these changes to happen really often - especially given that DOM changes will trigger it. It's not uncommon for people to do animations and other effects, and dynamic content updates too by adding/removing things from the dom. Clearing the IC chain when this happens, in Ion, might have an exaggerated performance impact on those kinds of pages.

I ended up scrapping most of what I had to focus on Ion as opposed to JM. Right now I'm making these uncacheable in JM. BTW, I was seeing the MAX_PIC_STUBS get hit often but it might be due to some bug in the IC (which we can hit pretty easily with expandos too). I'll file a separate bug about that. I've implemented the reset() approach for now with some logging. I'll try to do some testing with that, see how often we hit it in the real world. First I need to debug some crashes.

Peter: Are the interpreter changes already checked in (i.e. the pseudo shape stuff)? I'm asking because it may be worthwhile to just knock this off for baseline from the get go.

With changes to the baseline IC, it updates the generation in the IC if the guard fails.

Comment on attachment 743206 [details] [diff] [review] Codegen changes and make HTMLDocument OverrideBuiltin v1.1 This might need to change depending on how the review on the IC changes goes.

Comment on attachment 743206 [details] [diff] [review] Codegen changes and make HTMLDocument OverrideBuiltin v1.1 Can you move HasIdElementExposedAsHTMLDocumentProperty up higher in the file and use i in IdentifierMapEntryAddNames? For that matter, how about just making it an instance method on nsIdentifierMapEntry? >+ mozilla::dom::GenerationAndExpando mGenerationAndExpando; We discussed moving this struct's definition to jsfriendapi. Followup is fine. >+ CanHaveName(nsIAtom* aTag) I know you just copied this, but per spec this should include iframe. File a followup? And maybe a followup on the exposed object/embed thing.... >+ return v.isUndefined() ? NULL : &v.toObject(); nullptr? r=me

Comment on attachment 743203 [details] [diff] [review] JIT changes v2 Review of attachment 743203 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/BaselineIC.cpp @@ +3127,5 @@ > > // Check for list base if asked to. > RootedObject checkObj(cx, obj); > if (checkListBase && IsCacheableListBase(obj)) { > *checkListBase = isListBase = true; JS_ASSERT(listBaseHasGeneration) within conditional, mostly for documentation. @@ +5194,5 @@ > + if (listBaseHasGeneration) { > + Value expandoSlot = obj->getFixedSlot(GetListBaseExpandoSlot()); > + JS_ASSERT(!expandoSlot.isObject() && !expandoSlot.isUndefined()); > + void *internalStruct = expandoSlot.toPrivate(); > + for (ICStubConstIterator iter = stub->beginChainConst(); Refactor this check into a separate static function. e.g.: |bool UpdateExistingGenerationalListBaseStub(...)| ::: js/src/ion/IonCaches.cpp @@ +615,5 @@ > + > + if (!expandoVal.isObject() && !expandoVal.isUndefined()) { > + masm.branchTestValue(Assembler::NotEqual, tempVal, expandoVal, &failListBaseCheck); > + > + // Slot stores a private pointer to a struct with two fields, an int32 and a JS::Value. Since the jitcode that follows this accesses the structure, make it clear here exactly what the layout is expected to be, maybe even with the expected struct layout in comments: // Stores a private pointer to a struct that looks like : struct Foo { Value val; int32_t gen; }

Comment on attachment 743203 [details] [diff] [review] JIT changes v2 Review of attachment 743203 [details] [diff] [review]: ----------------------------------------------------------------- Nice, r=me with my and Kannan's comments addressed. ::: js/src/ion/BaselineIC.cpp @@ +307,5 @@ > + if (kind() == ICStub::GetProp_CallListBaseNative) { > + propStub = toGetProp_CallListBaseNative(); > + } else { > + propStub = toGetProp_CallListBaseWithGenerationNative(); > + } Nit: no { } @@ +3043,5 @@ > GenerateListBaseChecks(JSContext *cx, MacroAssembler &masm, Register object, > Address checkProxyHandlerAddr, > Address checkExpandoShapeAddr, > + Address* internalStructAddr, > + Address* generationAddr, Nit: Address *fooAddr (space before *) ::: js/src/ion/BaselineIC.h @@ +4321,5 @@ > + uint32_t pcOffset_; > + > + bool generateStubCode(MacroAssembler &masm, Address* internalStructAddr, > + Address* generationAddr); > + bool generateStubCode(MacroAssembler &masm) Nit: move this method into the cpp file. @@ +4325,5 @@ > + bool generateStubCode(MacroAssembler &masm) > + { > + if (kind == ICStub::GetProp_CallListBaseNative) { > + return generateStubCode(masm, NULL, NULL); > + } Nit: no {} @@ +4340,5 @@ > + ICStub *firstMonitorStub, HandleObject obj, > + HandleObject holder, HandleFunction getter, > + uint32_t pcOffset); > + > + ICStub *getStub(ICStubSpace *space) { Move this method to the cpp file too. @@ +4357,5 @@ > + JS_ASSERT(!expandoSlot.isObject() && !expandoSlot.isUndefined()); > + internalStruct = expandoSlot.toPrivate(); > + Value *expandoValPtr = (Value*)internalStruct; > + expandoVal = *expandoValPtr; > + generation = *(int32_t*)(expandoValPtr + 1); Would be nice if we could use the struct definition here (see the IonCaches review comment), or at least a comment similar to the IonCaches one. ::: js/src/ion/IonCaches.cpp @@ +616,5 @@ > + if (!expandoVal.isObject() && !expandoVal.isUndefined()) { > + masm.branchTestValue(Assembler::NotEqual, tempVal, expandoVal, &failListBaseCheck); > + > + // Slot stores a private pointer to a struct with two fields, an int32 and a JS::Value. > + void *internalStruct = expandoVal.toPrivate(); If I'm reading comment 15 correctly, GenerationAndExpando will be moved to jsfriendapi? If that's the case can we use "GenerationAndExpando *" here instead of "void *", and use its fields + offsetof below? @@ +1175,3 @@ > return true; > + if (shadows == DoesntShadowUnique) > + cache.reset(); Add a short comment here explaining why we reset it (to get rid of stubs for a previous counter value).

(In reply to Boris Zbarsky (:bz) from comment #15) > Can you move HasIdElementExposedAsHTMLDocumentProperty up higher in the file > and use i in IdentifierMapEntryAddNames? Done. > For that matter, how about just making it an instance method on > nsIdentifierMapEntry? Done. > We discussed moving this struct's definition to jsfriendapi. Followup is > fine. Done. > nullptr? Done. (In reply to Kannan Vijayan [:djvj] from comment #16) > JS_ASSERT(listBaseHasGeneration) within conditional, mostly for > documentation. Done. > Refactor this check into a separate static function. e.g.: > > |bool UpdateExistingGenerationalListBaseStub(...)| Done. > Since the jitcode that follows this accesses the structure, make it clear > here exactly what the layout is expected to be, maybe even with the expected > struct layout in comments: > > // Stores a private pointer to a struct that looks like : struct Foo { Value > val; int32_t gen; } Ended up moving the struct into jsfriendapi, so that the layout is actually fixed by the js engine. (In reply to Jan de Mooij [:jandem] from comment #17) > Nit: no { } Done. > Nit: Address *fooAddr (space before *) Done. > Nit: move this method into the cpp file. Done. > Nit: no {} Done. > Move this method to the cpp file too. Done. > Would be nice if we could use the struct definition here (see the IonCaches > review comment), or at least a comment similar to the IonCaches one. > If I'm reading comment 15 correctly, GenerationAndExpando will be moved to > jsfriendapi? If that's the case can we use "GenerationAndExpando *" here > instead of "void *", and use its fields + offsetof below? Done by moving the struct into jsfriendapi.h. > Add a short comment here explaining why we reset it (to get rid of stubs for > a previous counter value). Done. https://hg.mozilla.org/integration/mozilla-inbound/rev/6dba144500f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/2a999f8ee317

Backed out because of browser-chrome test failures: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a2245b038bcc https://hg.mozilla.org/integration/mozilla-inbound/rev/2b63238518a3

Should bug 841447 just be marked a duplicate of this?

As far as I can tell, the patches here were fine. Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/e604349e5a79 https://hg.mozilla.org/integration/mozilla-inbound/rev/9fc3d4e62179

https://hg.mozilla.org/mozilla-central/rev/e604349e5a79 https://hg.mozilla.org/mozilla-central/rev/9fc3d4e62179