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

src/runtime.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 5784 matching lines @@
   }
 
   // Allocate an array with storage for all the property names.
   Handle<FixedArray> names =
       isolate->factory()->NewFixedArray(total_property_count);
 
   // Get the property names.
   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++;
     }
     if (i < length - 1) {
       jsproto = Handle<JSObject>(JSObject::cast(jsproto->GetPrototype()));
     }
   }
 
-  // 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);
     }
   }
 
   return *isolate->factory()->NewJSArrayWithElements(names);
 }
 
 
 // Return the names of the local indexed properties.
 // args[0]: object
@@ skipping 9066 matching lines @@
     // Handle last resort GC and make sure to allow future allocations
     // to grow the heap without causing GCs (if possible).
     isolate->counters()->gc_last_resort_from_js()->Increment();
     isolate->heap()->CollectAllGarbage(Heap::kNoGCFlags,
                                        "Runtime::PerformGC");
   }
 }
 
 
 } }  // namespace v8::internal