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

PathTypeHandlerBase::SetPrototype changes hasNoEnumerableProperties of newPrototype #1622

Closed
mrkmarron opened this issue Sep 21, 2016 · 2 comments
Assignees
Labels
Milestone

Comments

@mrkmarron
Copy link
Contributor

In PathTypeHandler.cpp line 1470 has the call below which eventually ends up modifying the hasNoEnumerableProperties flag of the newPrototype object due to an "internal property" being added to the newPrototype object.

newPrototype->SetInternalProperty(Js::InternalPropertyIds::TypeOfPrototypeObjectDictionary, (Var)oldTypeToPromotedTypeMap, PropertyOperationFlags::PropertyOperation_Force, nullptr);

If the newPrototype object has hasNoEnumerableProperties as true, then this will call down through SimpleDictionaryTypeHandler.cpp line 1325 which adds the internal property with the default set of attribute flags:

return this->AddProperty(instance, propertyRecord, value, PropertyDynamicTypeDefaults, info, flags, SideEffects_Any);

This will then reach line 2609 which checks and sets hasNoEnumerableProperties as false:

if (attributes & PropertyEnumerable)
{
    instance->SetHasNoEnumerableProperties(false);
}
@mrkmarron
Copy link
Contributor Author

@kunalspathak: I first noticed it in relation to change #1489 "Type sharing for proto".

@kunalspathak kunalspathak self-assigned this Sep 21, 2016
@dilijev dilijev added the Bug label Sep 23, 2016
@curtisman curtisman added this to the 1.6 milestone May 17, 2017
@LouisLaf LouisLaf assigned rajatd and unassigned agarwal-sandeep Aug 14, 2017
@kunalspathak
Copy link
Contributor

@rajatd - While you are at it, please verify it for other internal properties as well.

@rajatd rajatd modified the milestones: 1.7, 1.6 Aug 17, 2017
chakrabot pushed a commit that referenced this issue Aug 26, 2017
…ttribute off.

Merge pull request #3587 from rajatd:hasNoEnumerableProps-2

While we don't enumerate internal properties, setting an internal property as enumerable unnecessarily sets the hasNoEnumerableProperties flag to false on the type handler. This may have perf impact as this flag is used to skip prototypes when enumerating properties.

Changing this to set internal properties as non-enumerable from the beginning. Doing this for non-path type handlers only because,
1. An object with a path type handler should theoretically have hasNoEnumerableProperties set to false anyway.
2. Setting a non-enumerable property on an object with a path type handler will convert its type handler to a (simple) dictionary type handler.

Fixes #1622
chakrabot pushed a commit that referenced this issue Aug 26, 2017
…enumerable" attribute off.

Merge pull request #3587 from rajatd:hasNoEnumerableProps-2

While we don't enumerate internal properties, setting an internal property as enumerable unnecessarily sets the hasNoEnumerableProperties flag to false on the type handler. This may have perf impact as this flag is used to skip prototypes when enumerating properties.

Changing this to set internal properties as non-enumerable from the beginning. Doing this for non-path type handlers only because,
1. An object with a path type handler should theoretically have hasNoEnumerableProperties set to false anyway.
2. Setting a non-enumerable property on an object with a path type handler will convert its type handler to a (simple) dictionary type handler.

Fixes #1622
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants