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.

Sign up for GitHub

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

Jump to bottom

ChakraRuntime swap back to native implementation for getPropertyNames #3527

Merged
3 commits merged into from
Oct 25, 2019
Merged

ChakraRuntime swap back to native implementation for getPropertyNames #3527

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b3e7a4d
go back to native implementation for getPropertyNames
stecrain Oct 25, 2019
3b72756
Change files
stecrain Oct 25, 2019
c658e38
disable object test
stecrain Oct 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions change/react-native-windows-2019-10-25-12-57-29-users-stecrain-fixChakraPerf.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"type": "prerelease",
"comment": "go back to native implementation for getPropertyNames",
"packageName": "react-native-windows",
"email": "[email protected]",
"commit": "b3e7a4d358874e8a6f693c8dc84c3022c7d4d087",
"date": "2019-10-25T19:57:29.289Z",
"file": "F:\\repos\\react-native-windows\\change\\react-native-windows-2019-10-25-12-57-29-users-stecrain-fixChakraPerf.json"
}
35 changes: 21 additions & 14 deletions vnext/JSI/Shared/ChakraRuntime.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,11 @@ namespace Microsoft::JSI {
namespace {

constexpr const char *const g_bootstrapBundleSource =
"function $$ChakraRuntimeGetPropertyNames$$(obj)\n"
"{\n"
" var propertyNames = []\n"
" for (propertyName in obj) \n"
" {\n"
" propertyNames.push(propertyName)\n"
" }\n"
" return propertyNames\n"
"}\n"
"function $$ChakraRuntimeProxyConstructor$$(target, handler)\n"
"{\n"
" return new Proxy(target, handler)\n"
"}";

constexpr const char *const g_getPropertyNamesBootstrapFuncName = "$$ChakraRuntimeGetPropertyNames$$";
constexpr const char *const g_proxyConstructorBootstrapFuncName = "$$ChakraRuntimeProxyConstructor$$";

constexpr const char *const g_proxyGetHostObjectTargetPropName = "$$ProxyGetHostObjectTarget$$";
Expand Down Expand Up @@ -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 •
edited
Loading

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.

facebook::jsi::Function jsGetPropertyNames =
global().getPropertyAsFunction(*this, g_getPropertyNamesBootstrapFuncName);
JsValueRef propertyNamesArrayRef;
VerifyJsErrorElseThrow(JsGetOwnPropertyNames(GetChakraObjectRef(object), &propertyNamesArrayRef));

JsPropertyIdRef propertyId;
VerifyJsErrorElseThrow(JsGetPropertyIdFromName(L"length", &propertyId));
JsValueRef countRef;
VerifyJsErrorElseThrow(JsGetProperty(propertyNamesArrayRef, propertyId, &countRef));
int count;

VerifyJsErrorElseThrow(JsNumberToInt(countRef, &count));

auto result = createArray(count);
for (int i = 0; i < count; i++) {
JsValueRef index;
VerifyJsErrorElseThrow(JsIntToNumber(i, &index));
JsValueRef propertyName;
VerifyJsErrorElseThrow(JsGetIndexedProperty(propertyNamesArrayRef, index, &propertyName));

facebook::jsi::Value objAsValue(*this, object);
return call(jsGetPropertyNames, facebook::jsi::Value::undefined(), &objAsValue, 1).asObject(*this).asArray(*this);
result.setValueAtIndex(*this, i, MakePointer<facebook::jsi::String>(propertyName));
}

return result;
}

// Only ChakraCore supports weak reference semantics, so ChakraRuntime
Expand Down