Speedup global_proxy.* attributes/accessors (specialize GlobalProxy access).

src/crankshaft/hydrogen.cc

Index: src/crankshaft/hydrogen.cc
diff --git a/src/crankshaft/hydrogen.cc b/src/crankshaft/hydrogen.cc
index da0ea4c318da50cde89c07f7e86b34a4b7ccf9f8..88189f36c9c54742761c128ed5513dcee1a47c71 100644
--- a/src/crankshaft/hydrogen.cc
+++ b/src/crankshaft/hydrogen.cc
@@ -5448,12 +5448,16 @@ void HOptimizedGraphBuilder::VisitConditional(Conditional* expr) {
   }
 }
 
+bool HOptimizedGraphBuilder::LookupGlobalPropertyCell(
+    Variable* var, LookupIterator* it, PropertyAccessType access_type) {
+  if (var->is_this()) return false;
+  return LookupGlobalPropertyCell(it, access_type);
+}
 
-HOptimizedGraphBuilder::GlobalPropertyAccess
-HOptimizedGraphBuilder::LookupGlobalProperty(Variable* var, LookupIterator* it,
-                                             PropertyAccessType access_type) {
-  if (var->is_this() || !current_info()->has_global_object()) {
-    return kUseGeneric;
+bool HOptimizedGraphBuilder::LookupGlobalPropertyCell(

[Toon Verwaest, 2016/09/27 14:57:12]

This should probably be called "CanInlineGlobalPropertyAccess"

[Alfonso, 2016/09/29 09:36:39]

Done.

+    LookupIterator* it, PropertyAccessType access_type) {
+  if (!current_info()->has_global_object()) {
+    return false;
   }
 
   switch (it->state()) {
@@ -5462,20 +5466,19 @@ HOptimizedGraphBuilder::LookupGlobalProperty(Variable* var, LookupIterator* it,
     case LookupIterator::INTERCEPTOR:
     case LookupIterator::INTEGER_INDEXED_EXOTIC:
     case LookupIterator::NOT_FOUND:
-      return kUseGeneric;
+      return false;
     case LookupIterator::DATA:
-      if (access_type == STORE && it->IsReadOnly()) return kUseGeneric;
-      if (!it->GetHolder<JSObject>()->IsJSGlobalObject()) return kUseGeneric;
-      return kUseCell;
+      if (access_type == STORE && it->IsReadOnly()) return false;
+      if (!it->GetHolder<JSObject>()->IsJSGlobalObject()) return false;
+      return true;
     case LookupIterator::JSPROXY:
     case LookupIterator::TRANSITION:
       UNREACHABLE();
   }
   UNREACHABLE();
-  return kUseGeneric;
+  return false;
 }
 
-
 HValue* HOptimizedGraphBuilder::BuildContextChainWalk(Variable* var) {
   DCHECK(var->IsContextSlot());
   HValue* context = environment()->context();
@@ -5488,6 +5491,55 @@ HValue* HOptimizedGraphBuilder::BuildContextChainWalk(Variable* var) {
   return context;
 }
 
+void HOptimizedGraphBuilder::LoadFromCell(LookupIterator* it,

[Toon Verwaest, 2016/09/27 14:57:11]

And InlineGlobalPropertyAccess

[Alfonso, 2016/09/29 09:36:40]

Done.

+                                          BailoutId ast_id) {
+  Handle<PropertyCell> cell = it->GetPropertyCell();
+  top_info()->dependencies()->AssumePropertyCell(cell);
+  auto cell_type = it->property_details().cell_type();
+  if (cell_type == PropertyCellType::kConstant ||
+      cell_type == PropertyCellType::kUndefined) {
+    Handle<Object> constant_object(cell->value(), isolate());
+    if (constant_object->IsConsString()) {
+      constant_object = String::Flatten(Handle<String>::cast(constant_object));
+    }
+    HConstant* constant = New<HConstant>(constant_object);
+    return ast_context()->ReturnInstruction(constant, ast_id);
+  } else {
+    auto access = HObjectAccess::ForPropertyCellValue();
+    UniqueSet<Map>* field_maps = nullptr;
+    if (cell_type == PropertyCellType::kConstantType) {
+      switch (cell->GetConstantType()) {
+        case PropertyCellConstantType::kSmi:
+          access = access.WithRepresentation(Representation::Smi());
+          break;
+        case PropertyCellConstantType::kStableMap: {
+          // Check that the map really is stable. The heap object could
+          // have mutated without the cell updating state. In that case,
+          // make no promises about the loaded value except that it's a
+          // heap object.
+          access = access.WithRepresentation(Representation::HeapObject());
+          Handle<Map> map(HeapObject::cast(cell->value())->map());
+          if (map->is_stable()) {
+            field_maps = new (zone())
+                UniqueSet<Map>(Unique<Map>::CreateImmovable(map), zone());
+          }
+          break;
+        }
+      }
+    }
+    HConstant* cell_constant = Add<HConstant>(cell);
+    HLoadNamedField* instr;
+    if (field_maps == nullptr) {
+      instr = New<HLoadNamedField>(cell_constant, nullptr, access);
+    } else {
+      instr = New<HLoadNamedField>(cell_constant, nullptr, access, field_maps,
+                                   HType::HeapObject());
+    }
+    instr->ClearDependsOnFlag(kInobjectFields);
+    instr->SetDependsOnFlag(kGlobalVars);
+    return ast_context()->ReturnInstruction(instr, ast_id);
+  }
+}
 
 void HOptimizedGraphBuilder::VisitVariableProxy(VariableProxy* expr) {
   DCHECK(!HasStackOverflow());
@@ -5536,57 +5588,8 @@ void HOptimizedGraphBuilder::VisitVariableProxy(VariableProxy* expr) {
       }
 
       LookupIterator it(global, variable->name(), LookupIterator::OWN);
-      GlobalPropertyAccess type = LookupGlobalProperty(variable, &it, LOAD);
-
-      if (type == kUseCell) {
-        Handle<PropertyCell> cell = it.GetPropertyCell();
-        top_info()->dependencies()->AssumePropertyCell(cell);
-        auto cell_type = it.property_details().cell_type();
-        if (cell_type == PropertyCellType::kConstant ||
-            cell_type == PropertyCellType::kUndefined) {
-          Handle<Object> constant_object(cell->value(), isolate());
-          if (constant_object->IsConsString()) {
-            constant_object =
-                String::Flatten(Handle<String>::cast(constant_object));
-          }
-          HConstant* constant = New<HConstant>(constant_object);
-          return ast_context()->ReturnInstruction(constant, expr->id());
-        } else {
-          auto access = HObjectAccess::ForPropertyCellValue();
-          UniqueSet<Map>* field_maps = nullptr;
-          if (cell_type == PropertyCellType::kConstantType) {
-            switch (cell->GetConstantType()) {
-              case PropertyCellConstantType::kSmi:
-                access = access.WithRepresentation(Representation::Smi());
-                break;
-              case PropertyCellConstantType::kStableMap: {
-                // Check that the map really is stable. The heap object could
-                // have mutated without the cell updating state. In that case,
-                // make no promises about the loaded value except that it's a
-                // heap object.
-                access =
-                    access.WithRepresentation(Representation::HeapObject());
-                Handle<Map> map(HeapObject::cast(cell->value())->map());
-                if (map->is_stable()) {
-                  field_maps = new (zone())
-                      UniqueSet<Map>(Unique<Map>::CreateImmovable(map), zone());
-                }
-                break;
-              }
-            }
-          }
-          HConstant* cell_constant = Add<HConstant>(cell);
-          HLoadNamedField* instr;
-          if (field_maps == nullptr) {
-            instr = New<HLoadNamedField>(cell_constant, nullptr, access);
-          } else {
-            instr = New<HLoadNamedField>(cell_constant, nullptr, access,
-                                         field_maps, HType::HeapObject());
-          }
-          instr->ClearDependsOnFlag(kInobjectFields);
-          instr->SetDependsOnFlag(kGlobalVars);
-          return ast_context()->ReturnInstruction(instr, expr->id());
-        }
+      if (LookupGlobalPropertyCell(variable, &it, LOAD)) {
+        return LoadFromCell(&it, expr->id());

[vogelheim, 2016/09/27 13:36:54]

LoadFromCell returns void. You *can* return a void-value, but I find that
confusing.

[Alfonso, 2016/09/29 09:36:40]

Done.

       } else {
         Handle<TypeFeedbackVector> vector(current_feedback_vector(), isolate());
         HLoadGlobalGeneric* instr = New<HLoadGlobalGeneric>(
@@ -6261,6 +6264,7 @@ bool HOptimizedGraphBuilder::PropertyAccessInfo::CanAccessMonomorphic() {
       name_.is_identical_to(isolate()->factory()->prototype_string())) {
     return IsLoad();
   }
+
   if (!LookupDescriptor()) return false;
   if (IsFound()) return IsLoad() || !IsReadOnly();
   if (IsIntegerIndexedExotic()) return false;
@@ -6722,8 +6726,7 @@ void HOptimizedGraphBuilder::HandleGlobalVariableAssignment(
   }
 
   LookupIterator it(global, var->name(), LookupIterator::OWN);
-  GlobalPropertyAccess type = LookupGlobalProperty(var, &it, STORE);
-  if (type == kUseCell) {
+  if (LookupGlobalPropertyCell(var, &it, STORE)) {
     Handle<PropertyCell> cell = it.GetPropertyCell();
     top_info()->dependencies()->AssumePropertyCell(cell);
     auto cell_type = it.property_details().cell_type();
@@ -7691,7 +7694,12 @@ HValue* HOptimizedGraphBuilder::BuildNamedAccess(
 
   if (maps->length() > 0) {
     PropertyAccessInfo info(this, access, maps->first(), name);
-    if (!info.CanAccessAsMonomorphic(maps)) {
+    bool can_access_mono = info.CanAccessAsMonomorphic(maps);

[vogelheim, 2016/09/27 13:36:54]

can_access_monomorphic

[Alfonso, 2016/09/29 09:36:40]

Done.

+    // TODO(peterssen): Add context access check.
+    // More than one map? Polymorphic.

[vogelheim, 2016/09/27 13:36:54]

This comment comments on one of the sub-clauses in the expression below. I'd
prefer if it would explain the statement intent. Maybe something like this:

// Check for special case: Access via a single map to the global proxy
// can also be handled monomorphically.
bool is_global_proxy_access = !can_access_monomorphic
  && maps->length() == 1
  && maps->first()->IsJSGlobalProxyMap();

[Alfonso, 2016/09/29 09:36:40]

Done.

+    bool is_global_proxy = maps->length() == 1 && !can_access_mono &&

[Toon Verwaest, 2016/09/27 14:57:11]

Why doesn't CanAccessMonomorphic just return true for the JSGlobalProxy case? It
*can* access it? ... that's essentailly what you're saying here.

Also, don't forget the compile-time access check. If that fails you won't be
able to inline the access.

[Alfonso, 2016/09/29 09:36:39]

CanAccessMonomorphic has many checks and some of them fail with the global
proxy, fixing them to handle the global proxy case is kind of useless since we
don't need the PropertyAccessInfo initialized to load from a cell anyway.

+                           maps->first()->IsJSGlobalProxyMap();
+    if (!can_access_mono && !is_global_proxy) {
       HandlePolymorphicNamedFieldAccess(access, expr, slot, ast_id, return_id,
                                         object, value, maps, name);
       return NULL;
@@ -7707,6 +7715,16 @@ HValue* HOptimizedGraphBuilder::BuildNamedAccess(
     } else {
       checked_object = Add<HCheckMaps>(object, maps);
     }
+
+    if (is_global_proxy) {
+      Handle<JSGlobalObject> global(current_info()->global_object());
+      LookupIterator it(global, name, LookupIterator::OWN);
+      if (LookupGlobalPropertyCell(&it, LOAD)) {
+        LoadFromCell(&it, expr->id());
+        return NULL;

[vogelheim, 2016/09/27 13:36:54]

[I don't understand Crankshaft at all, and am merely doing pattern-matching
here. Someone w/ better Crankshaft understanding should review this.]

This *looks* weird. So I take it nullptr is a valid return value; but I'm not
sure what that implies. Maybe LoadFromCell should be changed to return the instr
member. Then the use case in VisitVariableProxy could
ast_context()->ReturnInsttruction(LoadFromCell(..), ..), while this one would
return LoadFromCell(..)

Honestly, not sure what the right thing to do is.

[Alfonso, 2016/09/29 09:36:40]

To my understanding, returning NULL means that the instructions are already
pushed to the stream when the function returns. Returning the instruction allow
further manipulation by the caller.

+      }
+    }

[Toon Verwaest, 2016/09/27 14:57:11]

Doesn't this fall-through mean that you'll try to inline a monomorphic access to
a global proxy for which we couldn't inline the cell path, using a
ProperyAccessInfo that's totally not preparsed to do so?

[Alfonso, 2016/09/29 09:36:40]

Done.

+
     return BuildMonomorphicAccess(
         &info, object, checked_object, value, ast_id, return_id);
   }