-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement ChakraRuntime::getPropertyNames() properly in native. #3568
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. This is still a bit slower than the original implementation. But this is a good compromise to match the hermes behavior without tanking performance.
out of curiosity do we really need to match hermes here?
Also noticed something interesting. ChakraCore has an internal function GetOwnEnumerablePropertyNames which is used to implement the Object.keys property. Might be interesting to compare the performance of calling object.keys vs this approach where we need to call propertyIsEnumerable for each property name
https://github.com/microsoft/ChakraCore/blob/master/lib/Runtime/Library/JavascriptObject.cpp#L1072
// Host function and host object helpers | ||
// Convenience functions for property access. | ||
ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const ChakraObjectRef &id); | ||
inline ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const char *const name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i [](start = 0, length = 3)
(nit) can you add empty line here for readability? #Resolved
@@ -248,15 +239,35 @@ class ChakraRuntime : public facebook::jsi::Runtime { | |||
ChakraObjectRef ToChakraObjectRef(const facebook::jsi::Value &value); | |||
std::vector<ChakraObjectRef> ToChakraObjectRefs(const facebook::jsi::Value *value, size_t count); | |||
|
|||
// Host function and host object helpers | |||
// Convenience functions for property access. | |||
ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const ChakraObjectRef &id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id [](start = 81, length = 2)
(nit) just curious, why in one function this is "id" and in another it's "name" shouldn't they be consistent? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The difference is intentional as a JS property ID can either be backed by a Symbol or a name (a.k.a. string)
In reply to: 341261195 [](ancestors = 341261195)
// Host function and host object helpers | ||
// Convenience functions for property access. | ||
ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const ChakraObjectRef &id); | ||
inline ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const char *const name) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline [](start = 2, length = 6)
(nit) is this needed? compilers do inlining anyways #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea the compiler should automatically do inlining here, but I like the explicitness.
In reply to: 341261679 [](ancestors = 341261679)
// Convenience functions for property access. | ||
ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const ChakraObjectRef &id); | ||
inline ChakraObjectRef GetProperty(const ChakraObjectRef &obj, const char *const name) { | ||
return GetProperty(obj, GetChakraObjectRef(createPropNameIDFromAscii(name, strlen(name)))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createPropNameIDFromAscii [](start = 47, length = 25)
(nit) similarly, use "id" or "name" consistently. #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ChakraObjectRef objectPrototypePropertyIsEnumerable = GetProperty(objectPrototype, "propertyIsEnumerable"); | ||
|
||
std::vector<ChakraObjectRef> enumerablePropNames{}; | ||
ChakraObjectRef currentObject = GetChakraObjectRef(object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
object [](start = 53, length = 6)
(nit) naming. would it make more sense to call
facebook::jsi::object as "jsiObject"
?
#Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll rename currentObjec to currentObjectOnPrototypeChain.
In reply to: 341264449 [](ancestors = 341264449)
// Object.create(null). | ||
while (!CompareJsValues(currentObject, objectPrototype) && !CompareJsValues(currentObject, jsNull)) { | ||
JsValueRef propNamesRef = JS_INVALID_REFERENCE; | ||
VerifyJsErrorElseThrow(JsGetOwnPropertyNames(currentObject, &propNamesRef)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[](start = 63, length = 1)
(nit) please add /out/ #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need to add this to all sites where Chakra APIs are called. Punting this work for now.
In reply to: 341265034 [](ancestors = 341265034)
|
||
VerifyJsErrorElseThrow(JsNumberToInt(countRef, &count)); | ||
int numPropNames = ToInteger(GetProperty(propNames, "length")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numPropNames [](start = 8, length = 12)
(nit) propNamesLength? or propNamesSize? #Resolved
VerifyJsErrorElseThrow(JsGetIndexedProperty(propertyNamesArrayRef, index, &propertyName)); | ||
for (int i = 0; i < numPropNames; ++i) { | ||
JsValueRef propNameRef = JS_INVALID_REFERENCE; | ||
VerifyJsErrorElseThrow(JsGetIndexedProperty(propNames, ToJsNumber(i), &propNameRef)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[](start = 75, length = 1)
(nit) /out/ #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
VerifyJsErrorElseThrow(JsIntToNumber(i, &index)); | ||
JsValueRef propertyName; | ||
VerifyJsErrorElseThrow(JsGetIndexedProperty(propertyNamesArrayRef, index, &propertyName)); | ||
for (int i = 0; i < numPropNames; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for [](start = 4, length = 3)
(nit) can you please add a small comment describing what this loop does? We are getting propNames on "currentObject", but then we are switching "currentObject" inside of loop. #Resolved
currentObject = ChakraObjectRef(prototype); | ||
} | ||
|
||
size_t numEnumerablePropNames = enumerablePropNames.size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
numEnumerablePropNames [](start = 9, length = 22)
(nit) enumerablePropNamesSize or enumerablePropNamesLength #Resolved
|
||
int result = ToInteger(GetProperty(GetChakraObjectRef(arr), "length")); | ||
|
||
if (result < 0 || result > SIZE_MAX) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SIZE_MAX [](start = 29, length = 8)
(nit) use std::numeric_limits::max instead #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
if (result < 0 || result > SIZE_MAX) { | ||
throw facebook::jsi::JSINativeException("Invalid JS array length detected."); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) can we replace this with something like VerifyElseThrow(result < 0 || result > SIZE_MAX, "Invalid JS array length detected.") #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are other places that might benefit from a similar pattern. I think we should put this on hold till when we work on ensuring correct exception propagation.
In reply to: 341273515 [](ancestors = 341273515)
In #3514, we modified ChakraRuntime::getPropertyNames() so that it returns all non-enumerable properties along the property chain (this matches the behavior of HermesRuntime.) However, in doing so, we switched its implementation from native to JS. This introduced a massive performance regression, and we had to revert back to the old implementation (see #3527.)
This PR attempts to correct the native implementation of ChakraRuntime::getPropertyNames(). The new implementation is still slower due to the checking of enumerability.
Microsoft Reviewers: Open in CodeFlow