Skip to content
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

Use PathTypeHandler's for function objects with non-default properties/attributes #4818

Merged
merged 1 commit into from
Mar 21, 2018

Conversation

pleath
Copy link
Contributor

@pleath pleath commented Mar 14, 2018

Move from creating new non-shared SimpleTypeHandler's when a new property is added to a SimpleTypeHandler, or an attribute is changed, to creating PathTypeHandler's. Build a new root and evolve from the root to the current state of the SimpleTypeHandler, then replace the type's existing SimpleTypeHandler with the new PathTypeHandler. From there, we can evolve to the state required by the new property/attribute, and future instances of the same function that make the same type transition will get the same type. Also fix up some shared SimpleTypeHandler's in the library that didn't represent the desired state of their library objects and were forcing type transitions during initialization.

@pleath pleath force-pushed the funcpathtypes branch 2 times, most recently from 3411e00 to 24d7c5f Compare March 15, 2018 00:18
Copy link
Contributor

@sethbrenith sethbrenith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@@ -135,6 +112,64 @@ namespace Js
return newTypeHandler;
}

template<size_t size>
PathTypeHandlerBase* SimpleTypeHandler<size>::ConvertToPathType(DynamicObject* instance)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In one Speedometer test I saw a lot of SimpleTypeHandler::AddProperty/convert calls. This makes me think that SimpleTypeHandler type sharing isn't great (and so the PathTypeHandler sharing won't be great either). I'll try to find the case where this was happening, but I think there might be more to do here.

Is it the case that all function objects have their own SimpleTypeHandler?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see the code you're talking about. PathTypeHandler sharing will work in cases that SimpleTypeHandler doesn't -- basically, any case where we're changing the default properties/attributes of the object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see, this looks good! I'm excited to see the win we get!

@pleath
Copy link
Contributor Author

pleath commented Mar 15, 2018

@dotnet-bot test Windows ci_disablejit_x64_debug

…s/attributes. Move from creating new non-shared SimpleTypeHandler's when a new property is added to a SimpleTypeHandler, or an attribute is changed, to creating PathTypeHandler's. Build a new root and evolve from the root to the current state of the SimpleTypeHandler, then replace the type's existing SimpleTypeHandler with the new PathTypeHandler. From there, we can evolve to the state required by the new property/attribute, and future instances of the same function that make the same type transition will get the same type. Also fix up some shared SimpleTypeHandler's in the library that didn't represent the desired state of their library objects and were forcing type transitions during initialization.
@chakrabot chakrabot merged commit 978c171 into chakra-core:master Mar 21, 2018
chakrabot pushed a commit that referenced this pull request Mar 21, 2018
… non-default properties/attributes

Merge pull request #4818 from pleath:funcpathtypes

Move from creating new non-shared SimpleTypeHandler's when a new property is added to a SimpleTypeHandler, or an attribute is changed, to creating PathTypeHandler's. Build a new root and evolve from the root to the current state of the SimpleTypeHandler, then replace the type's existing SimpleTypeHandler with the new PathTypeHandler. From there, we can evolve to the state required by the new property/attribute, and future instances of the same function that make the same type transition will get the same type. Also fix up some shared SimpleTypeHandler's in the library that didn't represent the desired state of their library objects and were forcing type transitions during initialization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants