Bug 133683 – Inline caching should try to flatten uncacheable dictionaries

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 232793 [details]
Patch

[Geoffrey Garen]

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.

[Mark Hahnenberg]

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

[Geoffrey Garen]

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.

[Mark Hahnenberg]

Created attachment 232829 [details]
Patch

[Mark Hahnenberg]

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

[Filip Pizlo]

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.

[Gavin Barraclough]

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

[Geoffrey Garen]

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

[Geoffrey Garen]

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?

[Mark Hahnenberg]

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

[Geoffrey Garen]

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+.

[WebKit Commit Bot]

Comment on attachment 232829 [details]
Patch

Clearing flags on attachment: 232829

Committed r169853: <http://trac.webkit.org/changeset/169853>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.