Share SimpleTypeHandler types on function objects. - #4748
Conversation
| typeHandler->SetInstanceTypeHandler(instance); | ||
| if (functionProxy && typeHandler->GetMayBecomeShared()) | ||
| { | ||
| functionProxy->SetUndeferredFunctionType(ScriptFunction::FromVar(instance)->GetScriptFunctionType()); |
There was a problem hiding this comment.
ScriptFunction::FromVar(instance) [](start = 57, length = 33)
How do we know that instance is a ScriptFunction? (Not saying it's wrong, I'm just curious.)
There was a problem hiding this comment.
ScriptFunction's are the only JavascriptFunction's with non-null FunctionProxy.
There was a problem hiding this comment.
|
|
||
| friend class JavascriptArray; // for xplat offsetof field access | ||
| friend class JavascriptNativeArray; // for xplat offsetof field access | ||
| friend class JavascriptOperators; // for ReplaceType |
There was a problem hiding this comment.
Looks like the xplat builds are failing because JavascriptOperators is trying to access a protected member function DynamicObject::BoxStackInstance, so this friend wasn't just for ReplaceType
There was a problem hiding this comment.
True. So the different compilers are enforcing different rules?
c62c38a to
77e351e
Compare
|
|
||
| this->functionInfo = newFunctionInfo->GetFunctionInfo(); | ||
|
|
||
| if (type->GetIsShared()) |
There was a problem hiding this comment.
@curtisman If we're allowing shared types on function objects with non-default state, then we can't revert to the deferredPrototypeType here. I think we want to go ahead and change the entry point on the current shared type. Is there a reason we can't?
There was a problem hiding this comment.
My guess here is because originally, we would have swiitch out the functionInfo reference with the the undeferred function body in the scriptFunction. So undeferring would only be fully done for one script function but not the other instance. (Thus can't change the shared entry point)
Since you have changed it that the switching is done in the function proxy, it's probably OK to do.
But I wonder whether we need the line above any more as well. Shouldn't this->functionInfo should equals to newFunctionInfo->GetFunctionInfo() always?
In reply to: 171666256 [](ancestors = 171666256)
There was a problem hiding this comment.
I think you're right. I've replaced the copy with an assert. I think the only other thing we need to do is to copy the deferredPrototypeType and undeferredFunctionType pointers on un-defer/re-defer now that we don't make new types for the new FunctionProxy.
| if (undeferredFunctionType) | ||
| { | ||
| Assert(undeferredFunctionType->GetIsShared()); | ||
| instance->ReplaceType(undeferredFunctionType); |
There was a problem hiding this comment.
We should assert that undeferredFunctionType->GetTypeHandler() == typeHandler?
There was a problem hiding this comment.
I don't think we can do that. JavascriptLibrary::ConvertFunction calls this function with the shared SimpleTypeHandler from the library. That won't match the PathTypeHandler pointed to by undeferredFunctionType if we've done that conversion already.
| { | ||
| func(this->deferredPrototypeType); | ||
| } | ||
| if (this->undeferredFunctionType) |
There was a problem hiding this comment.
Since this is a path type, we should be able to enumerate those by walking that tree, instead of registering it to the functionObjectTypeList.
There was a problem hiding this comment.
This change isn't making the undeferredFunctionType into a path type. It's still a simple type. In any case, it will get registered as a function object type when we create it, so we probably don't need these lines.
|
@dotnet-bot test OSX static_osx_osx_debug |
| DynamicObject *protoObject = javascriptLibrary->CreateConstructorPrototypeObject(function); | ||
| if (scriptFunction && scriptFunction->GetFunctionInfo()->IsClassConstructor()) | ||
| { | ||
| function->SetPropertyWithAttributes(PropertyIds::prototype, protoObject, PropertyNone, nullptr); |
There was a problem hiding this comment.
SetPropertyWithAttributes [](start = 26, length = 25)
Unless I'm misreading something, isn't this changing the semantics? Previously we were only setting writable to false, now we'll be setting enumerable and configurable to false as well, no?
There was a problem hiding this comment.
We've always set the attributes this way for the prototype property of a class constructor. (See the class member defaults.)
45ffb1b to
f77daf6
Compare
|
|
88f006e to
2ad5e2b
Compare
…transition from DeferredTypeHandler to SimpleTypeHandler on first access to a function object's properties, we don't share the new type we create. Track an undeferred type along with the deferredPrototypeType on FunctionProxy and re-use it whenever possible for multiple instances of the same function. (This is a first step in broader sharing of function object types.)
2ad5e2b to
b748a70
Compare
Merge pull request #4748 from pleath:sharesimpletypes Currently when we transition from DeferredTypeHandler to SimpleTypeHandler on first access to a function object's properties, we don't share the new type we create. Track an undeferred type along with the deferredPrototypeType on FunctionProxy and re-use it whenever possible for multiple instances of the same function. (This is a first step in broader sharing of function object types.)
7 participants
Currently when we transition from DeferredTypeHandler to SimpleTypeHandler on first access to a function object's properties, we don't share the new type we create. Track an undeferred type along with the deferredPrototypeType on FunctionProxy and re-use it whenever possible for multiple instances of the same function. (This is a first step in broader sharing of function object types.)