Share SimpleTypeHandler types on function objects. #4748

pleath · 2018-02-27T22:37:08Z

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

sethbrenith · 2018-02-28T17:39:48Z

lib/Runtime/Types/DeferredTypeHandler.cpp

+            typeHandler->SetInstanceTypeHandler(instance);
+            if (functionProxy && typeHandler->GetMayBecomeShared())
+            {
+                functionProxy->SetUndeferredFunctionType(ScriptFunction::FromVar(instance)->GetScriptFunctionType());


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

ScriptFunction's are the only JavascriptFunction's with non-null FunctionProxy.

Could use ScriptFunction::UnsafeFromVar.

In reply to: 171357540 [](ancestors = 171357540)

Done. Thanks.

sethbrenith

MSLaguana · 2018-02-28T18:01:52Z

lib/Runtime/Types/DynamicObject.h

@@ -67,15 +67,8 @@ namespace Js

        friend class JavascriptArray; // for xplat offsetof field access
        friend class JavascriptNativeArray; // for xplat offsetof field access
-        friend class JavascriptOperators; // for ReplaceType


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

True. So the different compilers are enforcing different rules?

pleath · 2018-03-01T19:23:50Z

lib/Runtime/Library/ScriptFunction.cpp

        this->functionInfo = newFunctionInfo->GetFunctionInfo();

-        if (type->GetIsShared())


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

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)

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.

curtisman · 2018-03-02T13:47:17Z

lib/Runtime/Types/DeferredTypeHandler.cpp

+        if (undeferredFunctionType)
+        {
+            Assert(undeferredFunctionType->GetIsShared());
+            instance->ReplaceType(undeferredFunctionType);


We should assert that undeferredFunctionType->GetTypeHandler() == typeHandler?

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.

curtisman · 2018-03-02T13:50:53Z

lib/Runtime/Base/FunctionBody.cpp

@@ -2055,6 +2067,10 @@ namespace Js
        {
            func(this->deferredPrototypeType);
        }
+        if (this->undeferredFunctionType)


Since this is a path type, we should be able to enumerate those by walking that tree, instead of registering it to the functionObjectTypeList.

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.

pleath · 2018-03-02T22:04:59Z

@dotnet-bot test OSX static_osx_osx_debug

rajatd · 2018-03-08T23:09:51Z

lib/Runtime/Library/JavascriptLibrary.cpp

+            DynamicObject *protoObject = javascriptLibrary->CreateConstructorPrototypeObject(function);
+            if (scriptFunction && scriptFunction->GetFunctionInfo()->IsClassConstructor())
+            {
+                function->SetPropertyWithAttributes(PropertyIds::prototype, protoObject, PropertyNone, nullptr);


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?

We've always set the attributes this way for the prototype property of a class constructor. (See the class member defaults.)

rajatd · 2018-03-12T19:36:31Z

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

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

sethbrenith reviewed Feb 28, 2018

View reviewed changes

sethbrenith approved these changes Feb 28, 2018

View reviewed changes

MSLaguana reviewed Feb 28, 2018

View reviewed changes

pleath force-pushed the sharesimpletypes branch from c62c38a to 77e351e Compare February 28, 2018 19:34

boingoing approved these changes Mar 1, 2018

View reviewed changes

pleath force-pushed the sharesimpletypes branch from 77e351e to 0b25d43 Compare March 1, 2018 19:20

pleath commented Mar 1, 2018

View reviewed changes

curtisman reviewed Mar 2, 2018

View reviewed changes

pleath force-pushed the sharesimpletypes branch from 0b25d43 to 77bb304 Compare March 2, 2018 20:42

pleath force-pushed the sharesimpletypes branch from 77bb304 to 45ffb1b Compare March 7, 2018 18:57

rajatd reviewed Mar 8, 2018

View reviewed changes

pleath force-pushed the sharesimpletypes branch from 45ffb1b to f77daf6 Compare March 10, 2018 03:04

pleath force-pushed the sharesimpletypes branch 2 times, most recently from 88f006e to 2ad5e2b Compare March 13, 2018 00:36

pleath force-pushed the sharesimpletypes branch from 2ad5e2b to b748a70 Compare March 13, 2018 23:08

chakrabot merged commit b748a70 into chakra-core:master Mar 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share SimpleTypeHandler types on function objects. #4748

Share SimpleTypeHandler types on function objects. #4748

pleath commented Feb 27, 2018

sethbrenith Feb 28, 2018

pleath Feb 28, 2018

rajatd Mar 8, 2018

pleath Mar 12, 2018

sethbrenith left a comment

MSLaguana Feb 28, 2018

pleath Feb 28, 2018

pleath Mar 1, 2018

curtisman Mar 2, 2018

pleath Mar 2, 2018

curtisman Mar 2, 2018

pleath Mar 12, 2018

curtisman Mar 2, 2018

pleath Mar 5, 2018

pleath commented Mar 2, 2018

rajatd Mar 8, 2018

pleath Mar 12, 2018

rajatd commented Mar 12, 2018

		this->functionInfo = newFunctionInfo->GetFunctionInfo();

		if (type->GetIsShared())

Share SimpleTypeHandler types on function objects. #4748

Share SimpleTypeHandler types on function objects. #4748

Conversation

pleath commented Feb 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethbrenith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleath commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatd commented Mar 12, 2018