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

ChakraRuntime swap back to native implementation for getPropertyNames #3527

Merged
3 commits merged into from
Oct 25, 2019

Conversation

stecrain
Copy link
Contributor

@stecrain stecrain commented Oct 25, 2019

getPropertyNames was refactored to take advantage of javascript so that all property names up the prototype chain (this matches the hermes behavior).

When looking into page nav performance in the app I am working on I noticed getPropertyNames was showing up as much more expensive than in the past, also quite a bit more recycler thread activity.

With the JS implementation of getPropertyNames the duration from by button press to xaml beginning layout is 2.2 seconds.

image

Swapping back to a native implementation reduced the time from button press to xaml layout to 1 second, as overall gain of 1.2 seconds for the scenario.

CPU samples in getPropertyNames dropped from 615 to 25

image

Microsoft Reviewers: Open in CodeFlow

@stecrain stecrain requested a review from a team as a code owner October 25, 2019 20:12
@ghost ghost added the vnext label Oct 25, 2019
@stecrain stecrain changed the title Users/stecrain/fix chakra perf ChakraRuntime swap back to native implementation for getPropertyNames Oct 25, 2019
@@ -386,11 +376,28 @@ bool ChakraRuntime::isHostFunction(const facebook::jsi::Function &obj) const {
}

facebook::jsi::Array ChakraRuntime::getPropertyNames(const facebook::jsi::Object &object) {
Copy link
Contributor

@hansenyy hansenyy Oct 25, 2019

Choose a reason for hiding this comment

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

getPropertyNames [](start = 36, length = 16)

Since we are temporarily reverting back to the implementation that (incorrectly) returns non-enumerable properties, we fail JsiRuntimeUnitTests.ObjectTest at line 158 in JsiRuntimeUnitTests.cpp. We should temporarily disable this test as well. You can do so be prefixing DISABLED_ to the test name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hum It looks like the JSI unit tests are no longer being executed in our CI loop. I'll follow up on that.

Copy link
Contributor

@hansenyy hansenyy left a comment

Choose a reason for hiding this comment

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

:shipit:

@stecrain stecrain added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Oct 25, 2019
@ghost
Copy link

ghost commented Oct 25, 2019

Hello @stecrain!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 60 minutes, a condition that will be fulfilled in about 35 minutes. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@stecrain stecrain removed the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Oct 25, 2019
@stecrain stecrain added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Oct 25, 2019
@ghost ghost merged commit 911ea3c into microsoft:master Oct 25, 2019
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants