Optimize @@species based on a global 'protector' cell

src/lookup.cc

Index: src/lookup.cc
diff --git a/src/lookup.cc b/src/lookup.cc
index 83bfc79c8eefe6435b9134c33943e91f1d6bad83..dba0be0cae5480a0617888ab8296fe85562144da 100644
--- a/src/lookup.cc
+++ b/src/lookup.cc
@@ -142,6 +142,50 @@ void LookupIterator::ReloadPropertyInformation() {
   DCHECK(IsFound() || !holder_->HasFastProperties());
 }
 
+void LookupIterator::UpdateProtector() {
+  DisallowHeapAllocation no_gc;
+  if (IsElement()) return;
+  if (isolate_->bootstrapper()->IsActive()) return;
+  if (!isolate_->IsArraySpeciesLookupChainIntact()) return;
+
+  if (*name_ == *isolate_->factory()->constructor_string()) {
+    // Setting the constructor property could change an instance's @@species
+    if (holder_->IsJSArray()) {
+      isolate_->CountUsage(
+          v8::Isolate::UseCounterFeature::kArrayInstanceConstructorModified);
+      isolate_->InvalidateArraySpeciesProtector();
+    } else if (holder_->map()->is_prototype_map()) {
+      // Setting the constructor of Array.prototype of any realm also needs
+      // to invalidate the species protector
+      Object* context = heap()->native_contexts_list();
+      while (!context->IsUndefined()) {
+        Context* current_context = Context::cast(context);
+        if (current_context->initial_array_prototype() == *holder_) {
+          isolate_->CountUsage(v8::Isolate::UseCounterFeature::
+                                   kArrayPrototypeConstructorModified);
+          isolate_->InvalidateArraySpeciesProtector();
+          break;
+        }
+        context = current_context->get(Context::NEXT_CONTEXT_LINK);
+      }

[Camillo Bruni, 2016/02/22 12:44:47]

nit: Could you copy over and adapt the helper ContextForArrayOrObjectPrototype
from https://codereview.chromium.org/1409123003?
At least for me that made the intentions of this boilerplate code clearer.

Same below.

[Dan Ehrenberg, 2016/02/22 20:02:00]

Done

+    }
+  } else if (FLAG_harmony_species &&
+             *name_ == *isolate_->factory()->species_symbol()) {
+    // Setting the Symbol.species property of any Array constructor invalidates
+    // the species protector
+    Object* context = heap()->native_contexts_list();
+    while (!context->IsUndefined()) {
+      Context* current_context = Context::cast(context);
+      if (current_context->array_function() == *holder_) {

[Camillo Bruni, 2016/02/22 12:44:47]

you could check that the value we set is not the array_function (same above).
I could imagine that this would happen in a shim tying to emulate @@species on
an unsupported platform. In our case this would accidentally destroy
performance.

[Dan Ehrenberg, 2016/02/22 20:02:00]

Not really sure how I could do that. Polyfill libraries tend to set @@species
correctly, as a getter with a function, e.g.,
https://github.com/zloirock/core-js/blob/a6035a4b11b3e8522f8981b26c8ee6a20b2e...
. It seems rather complicated to determine if that getter is an identity
function--I don't know a clear way to do that. And it would be incorrect to
execute it just on Array and check if it equals that Array!

I think the strategy in this case would be to optimize the 'constructor' lookup,
and for the @@species lookup, make some sort of IC in the C++ code to speed up
the lookup.

[Camillo Bruni, 2016/02/22 20:05:22]

I guess this is fine for now. I thought of checking if you only set the value
directly on array_function and it corresponds to the array_function we simply
don't change the protector.
But indeed, there are other more complicated paths that could be covered, let's
move incrementally.

+        isolate_->CountUsage(
+            v8::Isolate::UseCounterFeature::kArraySpeciesModified);
+        isolate_->InvalidateArraySpeciesProtector();
+        break;
+      }
+      context = current_context->get(Context::NEXT_CONTEXT_LINK);
+    }
+  }
+}
 
 void LookupIterator::PrepareForDataProperty(Handle<Object> value) {
   DCHECK(state_ == DATA || state_ == ACCESSOR);