There exists a body of JS code that deletes properties off of objects (especially function/constructor objects), which puts them into an uncacheable dictionary state. This prevents all future inline caching for these objects. If properties are deleted out of the object during its initialization, we can enable caching for that object by attempting to flatten it when we see we're trying to do inline caching with that object. We then record that we performed this flattening optimization in the object's Structure. If it ever re-enters the uncacheable dictionary state then we can just give up on caching that object.
Created attachment 232793 [details] Patch
Comment on attachment 232793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232793&action=review r=me > Source/JavaScriptCore/jit/Repatch.cpp:727 > + if (structure->isUncacheableDictionary() && !structure->hasBeenFlattenedBefore()) > + asObject(baseCell)->flattenDictionaryObject(*vm); I think this line should precede the "Optimize self access" paragraph. If flattening works, there's no need to resort to building a polymorphic list right away.
(In reply to comment #2) > (From update of attachment 232793 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=232793&action=review > > r=me > > > Source/JavaScriptCore/jit/Repatch.cpp:727 > > + if (structure->isUncacheableDictionary() && !structure->hasBeenFlattenedBefore()) > > + asObject(baseCell)->flattenDictionaryObject(*vm); > > I think this line should precede the "Optimize self access" paragraph. If flattening works, there's no need to resort to building a polymorphic list right away. The problem with this approach is that flattening can potentially change the offset of the thing you just loaded. So we do it afterward so the next slow path will have the correct offset. We could also just re-do the lookup after flattening, but that seemed weirder to me.
Comment on attachment 232793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=232793&action=review >>> Source/JavaScriptCore/jit/Repatch.cpp:727 >>> + asObject(baseCell)->flattenDictionaryObject(*vm); >> >> I think this line should precede the "Optimize self access" paragraph. If flattening works, there's no need to resort to building a polymorphic list right away. > > The problem with this approach is that flattening can potentially change the offset of the thing you just loaded. So we do it afterward so the next slow path will have the correct offset. We could also just re-do the lookup after flattening, but that seemed weirder to me. I see. I guess this needs to go into buildList, then. That way, we'll do the flattening even in polymorphic cases.
Created attachment 232829 [details] Patch
(In reply to comment #5) > Created an attachment (id=232829) [details] > Patch I refactored some code to reduce duplication and make the logic of the inline caching stuff read more fluidly.
This seems like it can't possibly benefit anything other than microbenchmarks. If an object becomes an uncacheable dictionary then it gets a structure unique to it. Flattening the structure means that the structure is still unique to that one object. This means that this optimization only benefits expressions "o.f" where "o" is always the same object. This is probably very rare except in meaningless microbenchmarks. On the other hand this seems like it introduces non-trivial risk. So, I'm not sure we should do this.
(In reply to comment #7) > This seems like it can't possibly benefit anything other than microbenchmarks. If an object becomes an uncacheable dictionary then it gets a structure unique to it. Flattening the structure means that the structure is still unique to that one object. > > This means that this optimization only benefits expressions "o.f" where "o" is always the same object. This is probably very rare except in meaningless microbenchmarks. On the other hand this seems like it introduces non-trivial risk. > > So, I'm not sure we should do this. This seems like a valid concern, it would be great to try to establish how real websites use this to provide evidence one way or the other. I wonder whether we could add logging & gather stats on a real site that uses prototype – say, on apple.com? – are we repeatedly making accesses to uncachable dictionaries, and if so, is it the same one?
> This means that this optimization only benefits expressions "o.f" where "o" is always the same object. This is true of constructor and namespace objects, which are pretty common. In this benchmark, the object is a constructor.
I did some browsing around apple.com, and it didn't take long to get over 500 property delete by id actions. So, that tells us that delete is reasonably common -- but it doesn't tell us if these were constructor / namespace objects. Mark, can you set some breakpoints with this patch attached, and find out if the optimization ends up kicking in and sticking in a meaningful way?
(In reply to comment #10) > Mark, can you set some breakpoints with this patch attached, and find out if the optimization ends up kicking in and sticking in a meaningful way? After browsing around (for not very long), I found that apple.com triggers this optimization. I added some logging to tell us when we were triggering flattening (thus enabling inline caching) and when we were re-encountering an object that we had previously flattened but had re-entered uncacheable dictionary mode (thus causing us to give up on caching it). The code on apple.com seemed to have a high rate of success, meaning that we often able to flatten and we would rarely re-enter uncacheable dictionary mode. apple.com uses the prototype framework, which is also the portion of dromaeo that this patch helps the most. Using prototype all but guarantees that you will encounter this case in our inline caches because it does exactly what Geoff described above--it deletes some properties off of some of its constructor objects during initialization. Additionally, going to other sites like facebook.com also trigger this code path constantly. So overall, I think this is a worthwhile improvement. It may not be on the hottest of hot paths, but it's common enough and it prevents us from otherwise totally giving up on caching when we encounter an uncacheable dictionary.
Comment on attachment 232829 [details] Patch This data seems to resolve Phil's concern that this idiom only arises in meaningless microbenchmarks, so I'll say r+.
Comment on attachment 232829 [details] Patch Clearing flags on attachment: 232829 Committed r169853: <http://trac.webkit.org/changeset/169853>
All reviewed patches have been landed. Closing bug.