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

src/crankshaft/hydrogen.cc

 // Copyright 2013 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/crankshaft/hydrogen.h"
 
 #include <memory>
 #include <sstream>
 
 #include "src/allocation-site-scopes.h"
@@ skipping 5430 matching lines @@
 
   if (!ast_context()->IsTest()) {
     HBasicBlock* join = CreateJoin(cond_true, cond_false, expr->id());
     set_current_block(join);
     if (join != NULL && !ast_context()->IsEffect()) {
       return ast_context()->ReturnValue(Pop());
     }
   }
 }
 
+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()) {
     case LookupIterator::ACCESSOR:
     case LookupIterator::ACCESS_CHECK:
     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();
   int length = scope()->ContextChainLength(var->scope());
   while (length-- > 0) {
     context = Add<HLoadNamedField>(
         context, nullptr,
         HObjectAccess::ForContextSlot(Context::PREVIOUS_INDEX));
   }
   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());
   DCHECK(current_block() != NULL);
   DCHECK(current_block()->HasPredecessor());
   Variable* variable = expr->var();
   switch (variable->location()) {
     case VariableLocation::UNALLOCATED: {
       if (IsLexicalVariableMode(variable->mode())) {
         // TODO(rossberg): should this be an DCHECK?
@@ skipping 28 matching lines @@
             return Bailout(kReferenceToUninitializedVariable);
           }
           HInstruction* result = New<HLoadNamedField>(
               Add<HConstant>(script_context), nullptr,
               HObjectAccess::ForContextSlot(lookup.slot_index));
           return ast_context()->ReturnInstruction(result, expr->id());
         }
       }
 
       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>(
             variable->name(), ast_context()->typeof_mode(), vector,
             expr->VariableFeedbackSlot());
         return ast_context()->ReturnInstruction(instr, expr->id());
       }
     }
 
     case VariableLocation::PARAMETER:
@@ skipping 654 matching lines @@
 
 
 bool HOptimizedGraphBuilder::PropertyAccessInfo::CanAccessMonomorphic() {
   if (!CanInlinePropertyAccess(map_)) return false;
   if (IsJSObjectFieldAccessor()) return IsLoad();
   if (map_->IsJSFunctionMap() && map_->is_constructor() &&
       !map_->has_non_instance_prototype() &&
       name_.is_identical_to(isolate()->factory()->prototype_string())) {
     return IsLoad();
   }
+
   if (!LookupDescriptor()) return false;
   if (IsFound()) return IsLoad() || !IsReadOnly();
   if (IsIntegerIndexedExotic()) return false;
   if (!LookupInPrototypes()) return false;
   if (IsLoad()) return true;
 
   if (IsAccessorConstant()) return true;
   LookupTransition(*map_, *name_, NONE);
   if (IsTransitionToData() && map_->unused_property_fields() > 0) {
     // Construct the object field access.
@@ skipping 441 matching lines @@
           Add<HConstant>(script_context),
           HObjectAccess::ForContextSlot(lookup.slot_index), value);
       USE(instr);
       DCHECK(instr->HasObservableSideEffects());
       Add<HSimulate>(ast_id, REMOVABLE_SIMULATE);
       return;
     }
   }
 
   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();
     if (cell_type == PropertyCellType::kConstant ||
         cell_type == PropertyCellType::kUndefined) {
       Handle<Object> constant(cell->value(), isolate());
       if (value->IsConstant()) {
         HConstant* c_value = HConstant::cast(value);
         if (!constant.is_identical_to(c_value->handle(isolate()))) {
           Add<HDeoptimize>(DeoptimizeReason::kConstantGlobalVariableAssignment,
@@ skipping 947 matching lines @@
 HValue* HOptimizedGraphBuilder::BuildNamedAccess(
     PropertyAccessType access, BailoutId ast_id, BailoutId return_id,
     Expression* expr, FeedbackVectorSlot slot, HValue* object,
     Handle<Name> name, HValue* value, bool is_uninitialized) {
   SmallMapList* maps;
   ComputeReceiverTypes(expr, object, &maps, this);
   DCHECK(maps != NULL);
 
   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;
     }
 
     HValue* checked_object;
     // AstType::Number() is only supported by polymorphic load/call handling.
     DCHECK(!info.IsNumberType());
     BuildCheckHeapObject(object);
     if (AreStringTypes(maps)) {
       checked_object =
           Add<HCheckInstanceType>(object, HCheckInstanceType::IS_STRING);
     } 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);
   }
 
   return BuildNamedGeneric(access, expr, slot, object, name, value,
                            is_uninitialized);
 }
 
 
 void HOptimizedGraphBuilder::PushLoad(Property* expr,
@@ skipping 5568 matching lines @@
     isolate()->GetHTracer()->TraceHydrogen(name(), graph_);
   }
 
 #ifdef DEBUG
   graph_->Verify(false);  // No full verify.
 #endif
 }
 
 }  // namespace internal
 }  // namespace v8