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

Share types (i.e., allow PathTypeHandlers) for properties with non-standard attributes (non-writable, etc.). #4118

Merged
merged 1 commit into from
Nov 3, 2017

Conversation

pleath
Copy link
Contributor

@pleath pleath commented Nov 1, 2017

This change draws on Kount Veluri's work on behalf of native fields. It adds a wrinkle to the type path mechanism, replacing the PropertyId successor map key with a key that munges PropertyId and attributes. The way is paved for sharing types with getter/setters, deleted properties, and native values. When a property's attributes are changed/set, the type path is branched starting with the type that precedes the one that introduced the property.

@pleath pleath force-pushed the sharetypes-m branch 5 times, most recently from 85de59f to 5752595 Compare November 2, 2017 20:48
@@ -6,16 +6,62 @@

namespace Js
{
typedef uint8 SetterSlotIndex;
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe explicit naming this PathTypeHandlerSettingSlotIndex since this is only for PathTypeHandlers?

@@ -30,6 +76,9 @@ namespace Js
virtual BOOL IsLockable() const override { return true; }
virtual BOOL IsSharable() const override { return true; }

static PropertyAttributes ObjectSlotAttributesToPropertyAttributes(const ObjectSlotAttributes attr) { return attr & ObjectSlotAttr_PropertyAttributesMask; }
Copy link
Contributor

Choose a reason for hiding this comment

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

ObjectSlotAttributesToPropertyAttributes [](start = 34, length = 40)

Could we add static asserts to ensure property attributes have the same corresponding bits in PropertyAttributes and ObjectSlotAttributes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there are CompileAssert's in the cpp file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right.


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

DynamicType * GetPredecessorType() const { return predecessorType; }
PathTypeHandlerBase* GetRootPathTypeHandler();

virtual ObjectSlotAttributes * GetAttributeArray() const { return nullptr; }
virtual ObjectSlotAttributes GetAttributeArray(const PropertyIndex index) const { return ObjectSlotAttr_Default; }
Copy link
Contributor

Choose a reason for hiding this comment

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

GetAttributeArray [](start = 37, length = 17)

This is returning the attributes for a particular property index, right? If so, can we rename it to GetAttributes?

Proposing similar change for GetSetterSlots below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, right. That's what I had originally; didn't mean to change it. I'll change it back.

Copy link
Contributor

@curtisman curtisman left a comment

Choose a reason for hiding this comment

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

:shipit:

@pleath pleath force-pushed the sharetypes-m branch 2 times, most recently from 45a8241 to d28fde3 Compare November 2, 2017 23:53

if (instance->HasObjectArray())
{
requestContext->GetOrAddPropertyRecord(propertyName, propertyNameLength, &propertyRecord);
Copy link
Collaborator

Choose a reason for hiding this comment

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

use propertyNameString directly? #3875 adds PropertyString and LiteralStringWithPropertyStringPtr check.

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. This is actually a copy of some very old logic. Should we change it everywhere?

@obastemur
Copy link
Collaborator

LGTM

…andard attributes (non-writable, etc.). This change draws on Kount Veluri's work on behalf of native fields. It adds a wrinkle to the type path mechanism, replacing the PropertyId successor map key with a key that munges PropertyId and attributes. The way is paved for sharing types with getter/setters, deleted properties, and native values. When a property's attributes are changed/set, the type path is branched starting with the type that precedes the one that introduced the property.
@chakrabot chakrabot merged commit 2f81075 into chakra-core:master Nov 3, 2017
chakrabot pushed a commit that referenced this pull request Nov 3, 2017
…properties with non-standard attributes (non-writable, etc.).

Merge pull request #4118 from pleath:sharetypes-m

This change draws on Kount Veluri's work on behalf of native fields. It adds a wrinkle to the type path mechanism, replacing the PropertyId successor map key with a key that munges PropertyId and attributes. The way is paved for sharing types with getter/setters, deleted properties, and native values. When a property's attributes are changed/set, the type path is branched starting with the type that precedes the one that introduced the property.
IrinaYatsenko pushed a commit to IrinaYatsenko/ChakraCore that referenced this pull request Mar 9, 2018
IrinaYatsenko pushed a commit to IrinaYatsenko/ChakraCore that referenced this pull request Mar 9, 2018
… slots

Rename ThrowCantDelete and fix regression from chakra-core#4118 (delete non-config props in strict mode)
chakrabot pushed a commit that referenced this pull request Mar 9, 2018
… on TypedArray with missing slots

Merge pull request #4790 from irinayat-MS:Throw_copyWithin

Fixes #4356 and a regression from #4118
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.

5 participants