-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow type sharing for Object.create objects #3834
Conversation
@@ -653,6 +653,10 @@ namespace Js | |||
DynamicType::New(scriptContext, TypeIds_Object, objectPrototype, nullptr, typeHandler, true, true); | |||
} | |||
|
|||
SimplePathTypeHandler * typeHandler = SimplePathTypeHandler::New(scriptContext, this->GetRootPath(), 0, 0, sizeof(DynamicObject), true, true); | |||
typeHandler->SetIsInlineSlotCapacityLocked(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do, and why is it necessary? (Not a problem; I'm just asking for my own information.)
I'm referring specifically to the call to SetIsInlineSlotCapacityLocked
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I more or less blindly copied this from the normal ObjectType creation code. It prevents inline slot capacity from being reduced, and I think that actually doesn't do anything here because inline slot capacity is 0. Removed.
Field(RecyclerWeakReference<RecyclableObject>*) prototype; | ||
Field(RecyclerWeakReference<DynamicType>*) type; | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be possible to wrap the array of cache entries into a full struct/class? We'd get better encapsulation, and we could also move the cache lookup logic (lines 7296-7299 of JavascriptLibrary.cpp) into the cache implementation, again for better encapsulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@dotnet-bot test this please |
ef5e603
to
c557867
Compare
c557867
to
db778e3
Compare
RecyclableObject* cachedProto = cachedWeakProto->Get(); | ||
|
||
// We can use cache if the prototypes match and the Type hasn't been collected | ||
if (cachedProto == prototype && cachedType != nullptr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If cached type is null, consider just nulling out cache[index] so that those weakrefs can be collected (since that index is basically going to be a cache miss from now on)
DynamicType* newType = DynamicType::New(scriptContext, TypeIds_Object, prototype, nullptr, typeHandler, true, true); | ||
|
||
// Store the Type and prototype as weak references to avoid unnecessarily extending their lifetimes | ||
RecyclerWeakReference<RecyclableObject> * newWeakProtoHandle = prototype->GetRecycler()->CreateWeakReferenceHandle(prototype); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering how small this cache is, it's kind of nuts that we have to allocate 2 weak references (i.e 64 bytes) for every proto/pointer pair. I wonder if we can do something similar to inline caches here where we have a post mark cleanup step which simply scans this memory interpreting every pointer as a GC pointer and simply nulls it out if the item in question is not marked (and then have this cache be allocated as leaf instead)? Don't need to fix for this PR but worth thinking about
DynamicType* ObjectTypeCache::FindOrCreateType(_In_ RecyclableObject* prototype) | ||
{ | ||
// For fast lookup, cache index is a function of prototype address | ||
const uint index = (uint)(((uintptr_t)prototype) >> PolymorphicInlineCacheShift) & (ObjectTypeCache::MaxCachedTypes - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you collected any stats on how frequently this causes collisions? If prototype objects are similarly sized/allocated from the same heap block or so, we would see excessive collisions here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned that we already do some caching for prototype transitions, and I have a different change that will supersede this one. |
Merge pull request #3901 from MikeHolman:objcreateopt This is similar to my last iteration (#3834): Use the normal Object type if prototype is Object.prototype Use a new shared type for the case of null prototype (saved on javascriptLibrary) Difference is instead of using my new cache for other types, use the existing machinery in CreateObjectType
Object.create was always using null type handler, which meant that types could not be shared, which leads to all sorts of badness (e.g. our inline caches won't really work for these objects).
I've done a few things to make this better:
Use the normal Object type if prototype is Object.prototype
Use a new shared type for the case of null prototype (saved on javascriptLibrary)
Attempt to share types for other prototypes using a small global cache
Improves perf of Angular by about 25%.