Avoid duplication of a hidden & inherited prototype's properties.

src/runtime.cc

Index: src/runtime.cc
diff --git a/src/runtime.cc b/src/runtime.cc
index 8d84fdace2562f4f6f834940706f298bfdf481a5..52c9b910c5444b4ac32b506f54cacd747fbb2ed0 100644
--- a/src/runtime.cc
+++ b/src/runtime.cc
@@ -5802,8 +5802,32 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetLocalPropertyNames) {
   jsproto = obj;
   int proto_with_hidden_properties = 0;
   int next_copy_index = 0;
+  int duplicates = 0;
   for (int i = 0; i < length; i++) {
     jsproto->GetLocalPropertyNames(*names, next_copy_index, filter);
+    if (i > 0 && jsproto->map()->is_hidden_prototype()) {

[dcarney, 2013/12/26 13:26:58]

this should just be i > 0

[sof, 2013/12/26 21:48:45]

Done.

+      // Names from hidden prototypes may already have been added
+      // for inherited function template instances. Count the duplicates
+      // and stub them out; the final copy pass at the end ignores holes.
+      for (int j = next_copy_index;
+           j < next_copy_index + local_property_count[i];
+           j++) {
+        RUNTIME_ASSERT(names->get(j)->IsString());

[dcarney, 2013/12/24 08:48:08]

I don't think these asserts are necessary, but the correct assert would be
IsName(), since this function supports returning symbols as well as strings. 
also, an ASSERT would make more sense here

[sof, 2013/12/26 21:48:45]

Gone; I kept the use of String (rather than Name.)

+        String* name_from_hidden_proto = String::cast(names->get(j));
+        for (int k = 0; k < next_copy_index; k++) {
+          if (!names->get(k)->IsTheHole() &&

[dcarney, 2013/12/24 08:48:08]

no need for a hole check here, see last comment

+              names->get(k) != isolate->heap()->hidden_string()) {
+            RUNTIME_ASSERT(names->get(k)->IsString());
+            String* name = String::cast(names->get(k));
+            if (name_from_hidden_proto->Equals(name)) {

[dcarney, 2013/12/24 08:48:08]

all Name keys for properties are either symbols or internalized strings, so
Equals is overkill there, can just use == and merge the above if clause to make

name_from_hidden_proto == names->get(k) &&
isolate->heap()->hidden_string() != names->get(k)

+              names->set_the_hole(j);
+              duplicates++;
+              break;
+            }
+          }
+        }
+      }
+    }
     next_copy_index += local_property_count[i];
     if (jsproto->HasHiddenProperties()) {
       proto_with_hidden_properties++;
@@ -5813,17 +5837,17 @@ RUNTIME_FUNCTION(MaybeObject*, Runtime_GetLocalPropertyNames) {
     }
   }
 
-  // Filter out name of hidden properties object.
-  if (proto_with_hidden_properties > 0) {
+  // Filter out name of hidden properties object and
+  // hidden prototype duplicates.
+  if (proto_with_hidden_properties > 0 || duplicates > 0) {

[dcarney, 2013/12/24 08:48:08]

maybe merge these 2 variables into a single duplicate_properties variable

[dcarney, 2013/12/26 13:26:58]

actually, a better solution might be to call this unified variable
hidden_strings and insert the hidden string above instead of the hole. it would
reduce complexity

[sof, 2013/12/26 21:48:45]

Nice redux; done.

     Handle<FixedArray> old_names = names;
     names = isolate->factory()->NewFixedArray(
-        names->length() - proto_with_hidden_properties);
+        names->length() - proto_with_hidden_properties - duplicates);
     int dest_pos = 0;
     for (int i = 0; i < total_property_count; i++) {
       Object* name = old_names->get(i);
-      if (name == isolate->heap()->hidden_string()) {
-        continue;
-      }
+      if (name->IsTheHole()) continue;
+      if (name == isolate->heap()->hidden_string()) continue;
       names->set(dest_pos++, name);
     }
   }