Use PolymorphicInlineCache for PropertyString#2883
Conversation
MikeHolman
force-pushed
the
strpic_master
branch
4 times, most recently
from
28ff38f to
83d1fe5
Compare
MikeHolman
force-pushed
the
strpic_master
branch
from
83d1fe5 to
ebb5998
Compare
| class FunctionBodyPolymorphicInlineCache sealed : public PolymorphicInlineCache | ||
| { | ||
| private: | ||
| Field(FunctionBody *) functionBody; |
There was a problem hiding this comment.
I don't think you need to annotate class that are allocated as leaf.
| PropertyString * propertyString = (PropertyString *)Anew(arena, ArenaAllocPropertyString, type, propertyRecord); | ||
| propertyString->propCache = AllocatorNewStructZ(InlineCacheAllocator, type->GetScriptContext()->GetInlineCacheAllocator(), PropertyCache); | ||
| PropertyString * propertyString = RecyclerNewZ(recycler, PropertyString, type, propertyRecord); | ||
| propertyString->ldElemInlineCache = ScriptContextPolymorphicInlineCache::New(MinPropertyStringInlineCacheSize, type->GetScriptContext()); |
There was a problem hiding this comment.
Probably can inline these into property string to avoid the extra allocation?
There was a problem hiding this comment.
right now PIC don't ever grow, we just replace with a new one when we want to grow, so all the paths that deal with growing PIC would need to be updated.
| if (!PHASE_OFF1(Js::CloneCacheInCollisionPhase)) | ||
| { | ||
| if (!inlineCaches[inlineCacheIndex].IsEmpty() && !inlineCaches[inlineCacheIndex].NeedsToBeRegisteredForStoreFieldInvalidation()) | ||
| if (!inlineCaches[inlineCacheIndex].IsEmpty() && !inlineCaches[inlineCacheIndex].NeedsToBeRegisteredForStoreFieldInvalidation() && GetSize() != 1) |
There was a problem hiding this comment.
1 [](start = 160, length = 1)
is this '1' representing MinPropertyStringInlineCacheSize? If yes, could you use MinPropertyStringInlineCacheSize instead? Same in CacheProto, CacheAccessor, and CopyTo.
There was a problem hiding this comment.
It's really about the specific size value 1 rather than the abstract variable. Point is if there is only 1 slot, we can't clone to empty slot.
There was a problem hiding this comment.
| else if (currentSize == MinPropertyStringInlineCacheSize) | ||
| { | ||
| CompileAssert(MinPropertyStringInlineCacheSize < MinPolymorphicInlineCacheSize); | ||
| return MinPolymorphicInlineCacheSize; |
There was a problem hiding this comment.
MinPolymorphicInlineCacheSize [](start = 23, length = 29)
So, we're not gonna grow the cache?
There was a problem hiding this comment.
this case will grow cache from 1 to 4.
There was a problem hiding this comment.
Oh right, I misread MinPolymorphicInlineCacheSize and MinPropertyStringInlineCacheSize as the same
In reply to: 115339150 [](ancestors = 115339150)
| cache->GetInlineCaches()[cache->GetInlineCacheIndexForType(type)].Clear(); | ||
| } | ||
| cache = string->GetStElemInlineCache(); | ||
| if (cache->PretendTryGetProperty(type, &info)) |
There was a problem hiding this comment.
PretendTryGetProperty [](start = 27, length = 21)
Shouldn't this be a call to PretendTry-SET-Property ?
|
Can you guys take another look? I have more changes that I want to check in on top of this and don't have much time. |
|
Looks good to me. |
Merge pull request #2883 from MikeHolman:strpic_master PropertyStrings were earlier using their own special kind of inline caches. I have changed for them to now use our PolymorphicInlineCache, which can improve cases where we have polymorphic types., and also take advantage of existing InlineCache optimizations that weren't present in the specialized PropertyString cache. This gives about 8% improvement in react and 4% in angular.
5 participants
PropertyStrings were earlier using their own special kind of inline caches. I have changed for them to now use our PolymorphicInlineCache, which can improve cases where we have polymorphic types., and also take advantage of existing InlineCache optimizations that weren't present in the specialized PropertyString cache.
This gives about 8% improvement in react and 4% in angular.