Share types on objects with getter/setter properties. #4283

pleath · 2017-11-20T22:34:23Z

If the Accessor bit is set in a property's attributes, that indicates the property is a Getter. The Setter is stored in another slot. Use an array of indices to find the setter associated with a given Getter. Add the option of increasing a TypePath's path length without adding another property ID to its property map. Fix the way we use the accessor inline cache, in both runtime code and jitted code. We need to look at th isOnProto bit to determine which object to get the property from, the object pointed to by the cache (if isOnProto is set) or the current local instance. (This wasn't necessary when objects with accessors didn't share types.) While I was in there, I deleted all the machine-dependent lowering code related to flag caches and added code in Lower.cpp for everyone to share.

pleath · 2018-02-10T02:10:31Z

Updated this PR, merging in current state of master.

rajatd · 2018-02-14T21:56:25Z

    propertyOwnerType = localCache.u.accessor.object->GetType();

Missing check for isOnProto here? #Resolved

Refers to: lib/Backend/ObjTypeSpecFldInfo.cpp:303 in a848a47. [](commit_id = a848a47, deletion_comment = False)

rajatd · 2018-02-14T21:56:33Z

lib/Runtime/Language/InlineCache.inl

+            }
+            else
+            {
+                function = RecyclableObject::UnsafeFromVar(DynamicObject::FromVar(propertyObject)->GetInlineSlot(u.accessor.slotIndex));


FromVar [](start = 74, length = 7)

I think you can use UnsafeFromVar here since 'type' is the same as the cached type. #Resolved

Cool. Thanks. #Resolved

pleath · 2018-02-15T00:13:55Z

Good catch of "isOnProto." Thanks.

rajatd · 2018-02-15T19:06:19Z

lib/Runtime/Types/PathTypeHandler.cpp

+            {
+                PropertyIndex getterIndex = currentTypeHandler->GetPropertyIndex(currentPropertyRecord->GetPropertyId());
+                Assert(getterIndex != Constants::NoSlot);
+                if (currentTypeHandler->GetAttributes(getterIndex) & ObjectSlotAttr_Accessor)


currentTypeHandler->GetAttributes(getterIndex) & ObjectSlotAttr_Accessor [](start = 20, length = 72)

Wouldn't this condition be always true? #Resolved

Weirdly, no. In an object literal, for instance, you can initialize "get x", followed by a data property x, followed by "set x". #Resolved

rajatd · 2018-02-15T19:08:08Z

lib/Runtime/Types/PathTypeHandler.cpp

@@ -211,14 +211,24 @@ namespace Js
        while (true)
        {
            const PropertyRecord *currentPropertyRecord = GetTypePath()->GetPropertyIdUnchecked(currentSlotIndex);
-            currentType = currentTypeHandler->PromoteType<false>(currentType, PathTypeSuccessorKey(currentPropertyRecord->GetPropertyId(), currentAttributes), false, scriptContext, instance, &currentSlotIndex);
+            PropertyIndex index;
+            currentType = currentTypeHandler->PromoteType<false>(currentType, PathTypeSuccessorKey(currentPropertyRecord->GetPropertyId(), currentAttributes), false, scriptContext, instance, &index);


index [](start = 192, length = 5)

Call to InitializePath below was using the currentSlotIndex updated in PromoteType. Was that a bug all along? #Resolved

IIRC, the difference between currentSlotIndex and this local "index" only becomes possible when we start supporting accessors here, because of the extra slot we make for the setter. #Resolved

rajatd · 2018-02-15T20:24:28Z

lib/Runtime/Types/PathTypeHandler.cpp

+            }
+            if (getter != nullptr)
+            {
+                newTypeHandler->SetSlotUnchecked(instance, propertyIndex, CanonicalizeAccessor(getter, library));


SetSlotUnchecked [](start = 32, length = 16)

Would we possibly want to call SetSlotAndCache instead of SetSlotUnchecked?

We're not currently set up to use PropertyValueInfo on this path, so we can't use SetSlotAndCache. Currently DictionaryTypeHandler::SetAccessors is the only path for setting/adding accessors to a type handler, and it calls SetSlotUnchecked.

BTW, we're setting accessor inline caches at LdFld/StFld that call the accessors, so we're covered. We enter this method at defineProperty/object literal/etc., so setting an inline cache here isn't probably appropriate.

Agreed. But AddPropertyInternal in the 'if' clause above ends up calling SetSlotAndCache.

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

Discussed this offline. The gist is that in a non-add-property case the cache only needs to be updated when the type is changing, under SetAttributesHelper. We should investigate what we can do to make that path update the cache.

… bit is set in a property's attributes, that indicates the property is a Getter. The Setter is stored in another slot. Use an array of indices to find the setter associated with a given Getter. Add the option of increasing a TypePath's path length without adding another property ID to its property map. Fix the way we use the accessor inline cache, in both runtime code and jitted code. We need to look at th isOnProto bit to determine which object to get the property from, the object pointed to by the cache (if isOnProto is set) or the current local instance. (This wasn't necessary when objects with accessors didn't share types.) While I was in there, I deleted all the machine-dependent lowering code related to flag caches and added code in Lower.cpp for everyone to share.

…rties. Merge pull request #4283 from pleath:sharesetters If the Accessor bit is set in a property's attributes, that indicates the property is a Getter. The Setter is stored in another slot. Use an array of indices to find the setter associated with a given Getter. Add the option of increasing a TypePath's path length without adding another property ID to its property map. Fix the way we use the accessor inline cache, in both runtime code and jitted code. We need to look at th isOnProto bit to determine which object to get the property from, the object pointed to by the cache (if isOnProto is set) or the current local instance. (This wasn't necessary when objects with accessors didn't share types.) While I was in there, I deleted all the machine-dependent lowering code related to flag caches and added code in Lower.cpp for everyone to share.

pleath force-pushed the sharesetters branch 3 times, most recently from 3789e94 to 92a20f1 Compare November 21, 2017 22:40

pleath force-pushed the sharesetters branch from 92a20f1 to 4be3a9e Compare February 10, 2018 02:09

pleath requested review from rajatd, curtisman and LouisLaf February 10, 2018 02:10

pleath force-pushed the sharesetters branch 2 times, most recently from 0a49a2b to a848a47 Compare February 11, 2018 02:08

rajatd reviewed Feb 14, 2018

View reviewed changes

pleath force-pushed the sharesetters branch from a848a47 to 4ef52b6 Compare February 15, 2018 00:36

rajatd reviewed Feb 15, 2018

View reviewed changes

pleath force-pushed the sharesetters branch from 4ef52b6 to 83f8af8 Compare February 21, 2018 01:36

chakrabot merged commit 83f8af8 into chakra-core:master Feb 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share types on objects with getter/setter properties. #4283

Share types on objects with getter/setter properties. #4283

pleath commented Nov 20, 2017

pleath commented Feb 10, 2018

rajatd commented Feb 14, 2018 •

edited

Loading

rajatd Feb 14, 2018 •

edited

Loading

pleath Feb 15, 2018 •

edited by rajatd

Loading

pleath commented Feb 15, 2018

rajatd Feb 15, 2018 •

edited

Loading

pleath Feb 15, 2018 •

edited by rajatd

Loading

rajatd Feb 15, 2018 •

edited

Loading

pleath Feb 15, 2018 •

edited by rajatd

Loading

rajatd Feb 15, 2018

pleath Feb 20, 2018

pleath Feb 20, 2018

rajatd Feb 20, 2018

pleath Feb 21, 2018

Share types on objects with getter/setter properties. #4283

Share types on objects with getter/setter properties. #4283

Conversation

pleath commented Nov 20, 2017

pleath commented Feb 10, 2018

rajatd commented Feb 14, 2018 • edited Loading

rajatd Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

pleath Feb 15, 2018 • edited by rajatd Loading

Choose a reason for hiding this comment

pleath commented Feb 15, 2018

rajatd Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

pleath Feb 15, 2018 • edited by rajatd Loading

Choose a reason for hiding this comment

rajatd Feb 15, 2018 • edited Loading

Choose a reason for hiding this comment

pleath Feb 15, 2018 • edited by rajatd Loading

Choose a reason for hiding this comment

rajatd Feb 15, 2018

Choose a reason for hiding this comment

pleath Feb 20, 2018

Choose a reason for hiding this comment

pleath Feb 20, 2018

Choose a reason for hiding this comment

rajatd Feb 20, 2018

Choose a reason for hiding this comment

pleath Feb 21, 2018

Choose a reason for hiding this comment

rajatd commented Feb 14, 2018 •

edited

Loading

rajatd Feb 14, 2018 •

edited

Loading

pleath Feb 15, 2018 •

edited by rajatd

Loading

rajatd Feb 15, 2018 •

edited

Loading

pleath Feb 15, 2018 •

edited by rajatd

Loading

rajatd Feb 15, 2018 •

edited

Loading

pleath Feb 15, 2018 •

edited by rajatd

Loading