Sitelet https://github.com/chakra-core/ChakraCore/pull/1449
Skip to content

Fastpath for hasOwnProperty == true. - #1449

Merged
chakrabot merged 1 commit into
chakra-core:masterfrom
dilijev:hasown
Aug 17, 2016
Merged

chakrabot merged 1 commit into
chakra-core:masterfrom
dilijev:hasown

Conversation

@dilijev

@dilijev dilijev commented Aug 16, 2016 •

Copy link
Copy Markdown
Contributor

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 commented Aug 16, 2016

Copy link
Copy Markdown
Contributor Author

@curtisman @agarwal-sandeep please review?

@dilijev

dilijev commented Aug 16, 2016

Copy link
Copy Markdown
Contributor Author

@dotnet-bot test Windows arm_test please

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

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

@agarwal-sandeep agarwal-sandeep Aug 16, 2016 •

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

ljharb commented Aug 16, 2016

Copy link
Copy Markdown
Collaborator

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

dilijev commented Aug 16, 2016

Copy link
Copy Markdown
Contributor Author

@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

ljharb commented Aug 16, 2016

Copy link
Copy Markdown
Collaborator

awesome, thanks :-)

@agarwal-sandeep

agarwal-sandeep commented Aug 17, 2016 •

Copy link
Copy Markdown
Collaborator
        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)

Comment thread lib/Runtime/Base/ScriptContext.cpp Outdated
PropertyString* ScriptContext::GetPropertyString(PropertyId propertyId)
{
PropertyString *string = TryGetPropertyString(propertyId);
if (string)

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator

:shipit:

@dilijev

dilijev commented Aug 17, 2016

Copy link
Copy Markdown
Contributor Author

@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