[MERGE chakra-core#2298 @ThomsonTan] Remove downward lookup when searching shadowed property in prototype chain in for...in

ThomsonTan · ThomsonTan · commit 6a8a1293e7df · 2017-01-10T14:35:02.000-08:00
Merge pull request chakra-core#2298 from ThomsonTan:FixShadowPropertyInForIn The previous fix of shadowing non-enumerable property for for...in (chakra-core#2175) added downward lookup in prototype chain which could degrade performance in edge cases. This fix followed suggestion from Curtis and only ignores lookup in prototype chain if there is no enumerable properties in the whole chain, otherwise search from the first prototype. The perf data for which we optimized for in enumerator looks flat.
diff --git a/lib/Runtime/Library/ForInObjectEnumerator.cpp b/lib/Runtime/Library/ForInObjectEnumerator.cpp
@@ -11,14 +11,11 @@ namespace Js
     ForInObjectEnumerator::ShadowData::ShadowData(
         RecyclableObject * initObject,
         RecyclableObject * firstPrototype,
-        RecyclableObject * firstPrototypeWithEnumerableProperties,
         Recycler * recycler)
         : currentObject(initObject),
           firstPrototype(firstPrototype),
-          firstPrototypeWithEnumerableProperties(firstPrototypeWithEnumerableProperties),
           propertyIds(recycler)
     {
-
     }
 
     ForInObjectEnumerator::ForInObjectEnumerator(RecyclableObject* object, ScriptContext * scriptContext, bool enumSymbols)
@@ -53,9 +50,10 @@ namespace Js
         if (firstPrototypeWithEnumerableProperties != nullptr)
         {
             Recycler *recycler = requestContext->GetRecycler();
-            this->shadowData = RecyclerNew(recycler, ShadowData, initObject, firstPrototype, firstPrototypeWithEnumerableProperties, recycler);
+            this->shadowData = RecyclerNew(recycler, ShadowData, initObject, firstPrototype, recycler);
             flags = EnumeratorFlags::UseCache | EnumeratorFlags::SnapShotSemantics | EnumeratorFlags::EnumNonEnumerable | (enumSymbols ? EnumeratorFlags::EnumSymbols : EnumeratorFlags::None);
         }
+        // no enumerable properties in the prototype chain, no need to search it
         else
         {
             this->shadowData = nullptr;
@@ -203,36 +201,10 @@ namespace Js
                     }
                 }
 
-                //check for shadowed property
                 if (TestAndSetEnumerated(propertyId) //checks if the property is already enumerated or not
                     && (attributes & PropertyEnumerable))
                 {
-                    bool propertyShadowed = false;
-
-                    if (this->enumeratingPrototype)
-                    {
-                        // prototype checking begins from the first prototype object with enumerable properties,
-                        // but the property could be shadowed by a desendant prototype which has the same property but not enumerable.
-                        // Need to check that because that is ignored from the begining.
-                        RecyclableObject * prototypeObject = this->shadowData->firstPrototype;
-
-                        while (prototypeObject != nullptr && prototypeObject != this->shadowData->currentObject)
-                        {
-                            if (prototypeObject->HasProperty(propertyId))
-                            {
-                                propertyShadowed = true;
-                                break;
-                            }
-                            prototypeObject = prototypeObject->GetPrototype();
-
-                            Assert(prototypeObject != nullptr);
-                        }
-                    }
-
-                    if (!propertyShadowed)
-                    {
-                        return currentIndex;
-                    }
+                    return currentIndex;
                 }
             }
             else
@@ -244,10 +216,10 @@ namespace Js
                 }
 
                 RecyclableObject * object;
-                if (!enumeratingPrototype)
+                if (!this->enumeratingPrototype)
                 {  
                     this->enumeratingPrototype = true;
-                    object = this->shadowData->firstPrototypeWithEnumerableProperties;
+                    object = this->shadowData->firstPrototype;
                     this->shadowData->currentObject = object;
                 }
                 else
diff --git a/lib/Runtime/Library/ForInObjectEnumerator.h b/lib/Runtime/Library/ForInObjectEnumerator.h
@@ -12,10 +12,9 @@ namespace Js
         JavascriptStaticEnumerator enumerator;
         struct ShadowData
         {
-            ShadowData(RecyclableObject * initObject, RecyclableObject * firstPrototype, RecyclableObject * firstPrototypeWithEnumerableProperties, Recycler * recycler);
+            ShadowData(RecyclableObject * initObject, RecyclableObject * firstPrototype, Recycler * recycler);
             RecyclableObject * currentObject;
             RecyclableObject * firstPrototype;
-            RecyclableObject * firstPrototypeWithEnumerableProperties;
             BVSparse<Recycler> propertyIds;
             SListBase<Js::PropertyRecord const *> newPropertyStrings;
         } *shadowData;
