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

Fastpath for hasOwnProperty == true. #1449

Merged
merged 1 commit into from
Aug 17, 2016
Merged

Conversation

dilijev
Copy link
Contributor

@dilijev dilijev commented Aug 16, 2016

Adds a fastpath for the case when Object.hasOwnProperty returns true (meaning the property was found on the current object).

This optimization was inspired by the observation that a very common pattern to use Object.hasOwnProperty in the following manner:

for (var x in obj) {
    if (obj.hasOwnProperty(x)) {
        // do something
    }
}

From our observations, most instances of these patterns are used with shallow objects (objects with most of their enumerable properties on the object itself, rather than in the prototype chain), so most of the time, Object.hasOwnProperty would return true when invoked via the above pattern. Additionally, we have already created a property cache containing the type that a property was enumerated from, so we can use that information to trivially know whether the this parameter of hasOwnProperty was the same object on which we looked up that property, and if so, we know the answer is true.

@dilijev
Copy link
Contributor Author

dilijev commented Aug 16, 2016

@curtisman @agarwal-sandeep please review?

@dilijev
Copy link
Contributor Author

dilijev commented Aug 16, 2016

@dotnet-bot test Windows arm_test please

BOOL expectedAnswer = object && object->HasOwnProperty(propertyId);
#endif

PropertyString *propString = requestContext->GetPropertyString(propertyId);
Copy link
Collaborator

@agarwal-sandeep agarwal-sandeep Aug 16, 2016

Choose a reason for hiding this comment

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

GetPropertyString [](start = 61, length = 17)

This goes and create a PropertyString if one is not present in that case type below won't match, should we have a different function which returns only if PropertyString is present. #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great catch.


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

@ljharb
Copy link
Collaborator

ljharb commented Aug 16, 2016

I'm not sure I'm understanding what's going on in this PR, but could this also add a fast path for Object.prototype.hasOwnProperty.call(obj, prop)?

@dilijev
Copy link
Contributor Author

dilijev commented Aug 16, 2016

@ljharb This code is the builtin for the function Object.hasOwnProperty, so we will always get this advantage no matter how that function is invoked. (I'll add a test case just to be sure.)

I'll add some more information above about what this change accomplishes.

@ljharb
Copy link
Collaborator

ljharb commented Aug 16, 2016

awesome, thanks :-)

@agarwal-sandeep
Copy link
Collaborator

agarwal-sandeep commented Aug 17, 2016

        string = stringReference->Get();

Move PropertyString *string; from above to here, as we don't need it if propertyStringMap->TryGetValue fails #Resolved


Refers to: lib/Runtime/Base/ScriptContext.cpp:1465 in c1e972d. [](commit_id = c1e972d, deletion_comment = False)

PropertyString* ScriptContext::GetPropertyString(PropertyId propertyId)
{
PropertyString *string = TryGetPropertyString(propertyId);
if (string)
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (string) [](start = 8, length = 11)

if (string != nullptr)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done!

@agarwal-sandeep
Copy link
Collaborator

:shipit:

@dilijev
Copy link
Contributor Author

dilijev commented Aug 17, 2016

@dotnet-bot test Windows x64_test please

@chakrabot chakrabot merged commit c1e972d into chakra-core:master Aug 17, 2016
chakrabot pushed a commit that referenced this pull request Aug 17, 2016
Merge pull request #1449 from dilijev:hasown

Adds a fastpath for the case when `Object.hasOwnProperty` returns `true` (meaning the property was found on the current object).

This optimization was inspired by the observation that a very common pattern to use `Object.hasOwnProperty` in the following manner:

```
for (var x in obj) {
    if (obj.hasOwnProperty(x)) {
        // do something
    }
}
```

From our observations, most instances of these patterns are used with shallow objects (objects with most of their enumerable properties on the object itself, rather than in the prototype chain), so most of the time, `Object.hasOwnProperty` would return true when invoked via the above pattern. Additionally, we have already created a property cache containing the type that a property was enumerated from, so we can use that information to trivially know whether the `this` parameter of `hasOwnProperty` was the same object on which we looked up that property, and if so, we know the answer is `true`.
@dilijev dilijev deleted the hasown branch August 17, 2016 00:49
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.

6 participants