Optimize Function.prototype.call

src/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/hydrogen.h"
 
 #include <algorithm>
 
 #include "src/v8.h"
 #include "src/allocation-site-scopes.h"
@@ skipping 7957 matching lines @@
       }
       break;
     default:
       // Not supported for inlining yet.
       break;
   }
   return false;
 }
 
 
 bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(

[Toon Verwaest, 2014/06/12 21:41:13]

Does it make sense to have both methods? I'm pretty sure that by having both,
people will start calling the other version, which may not be correct?

[p.antonov, 2014/06/13 08:34:24]

Done.

     Call* expr,
     HValue* receiver,
-    Handle<Map> receiver_map) {
+    Handle<Map> map,
+    Handle<JSFunction> function,
+    int args_count_no_receiver
+) {

[Toon Verwaest, 2014/06/12 21:41:13]

Nit: put ) { back on the previous line

[p.antonov, 2014/06/13 08:34:24]

Done.

+  if (!function->shared()->HasBuiltinFunctionId()) return false;
+  BuiltinFunctionId id = function->shared()->builtin_function_id();
+  if (InlineBuiltinMethodCall(expr, receiver, map, id,
+                                args_count_no_receiver)) {
+    if (FLAG_trace_inlining) {
+      PrintF("Inlining builtin ");
+      function->ShortPrint();
+      PrintF("\n");
+    }
+    return true;
+  }
+  return false;
+}
+
+
+bool HOptimizedGraphBuilder::InlineBuiltinMethodCall(
+    Call* expr,
+    HValue* receiver,
+    Handle<Map> receiver_map,
+    BuiltinFunctionId id,
+    //  expr could be .call or .apply so it's not safe to rely on its

[Toon Verwaest, 2014/06/12 21:41:13]

I don't think we need this comment...

[p.antonov, 2014/06/13 08:34:24]

Done.

+    //  ->arguments()->length()
+    int args_count_no_receiver) {
+  int argument_count = args_count_no_receiver + 1;  // Plus receiver.
   // Try to inline calls like Math.* as operations in the calling function.
-  if (!expr->target()->shared()->HasBuiltinFunctionId()) return false;
-  BuiltinFunctionId id = expr->target()->shared()->builtin_function_id();
-  int argument_count = expr->arguments()->length() + 1;  // Plus receiver.
   switch (id) {
     case kStringCharCodeAt:
     case kStringCharAt:
       if (argument_count == 2) {
         HValue* index = Pop();
         HValue* string = Pop();
         Drop(1);  // Function.
         HInstruction* char_code =
             BuildStringCharCodeAt(string, index);
         if (id == kStringCharCodeAt) {
@@ skipping 87 matching lines @@
       }
       break;
     case kArrayPop: {
       if (receiver_map.is_null()) return false;
       if (receiver_map->instance_type() != JS_ARRAY_TYPE) return false;
       ElementsKind elements_kind = receiver_map->elements_kind();
       if (!IsFastElementsKind(elements_kind)) return false;
       if (receiver_map->is_observed()) return false;
       ASSERT(receiver_map->is_extensible());
 
-      Drop(expr->arguments()->length());
+      Drop(args_count_no_receiver);
       HValue* result;
       HValue* reduced_length;
       HValue* receiver = Pop();
 
       HValue* checked_object = AddCheckMap(receiver, receiver_map);
       HValue* length = Add<HLoadNamedField>(
           checked_object, static_cast<HValue*>(NULL),
           HObjectAccess::ForArrayLength(elements_kind));
 
       Drop(1);  // Function.
@@ skipping 55 matching lines @@
       // inlined version can't be used.
       if (receiver_map->DictionaryElementsInPrototypeChainOnly()) return false;
       // If there currently can be no elements accessors on the prototype chain,
       // it doesn't mean that there won't be any later. Install a full prototype
       // chain check to trap element accessors being installed on the prototype
       // chain, which would cause elements to go to dictionary mode and result
       // in a map change.
       Handle<JSObject> prototype(JSObject::cast(receiver_map->prototype()));
       BuildCheckPrototypeMaps(prototype, Handle<JSObject>());
 
-      const int argc = expr->arguments()->length();
+      const int argc = args_count_no_receiver;
       if (argc != 1) return false;
 
       HValue* value_to_push = Pop();
       HValue* array = Pop();
       Drop(1);  // Drop function.
 
       HInstruction* new_size = NULL;
       HValue* length = NULL;
 
       {
@@ skipping 36 matching lines @@
       // chain check to trap element accessors being installed on the prototype
       // chain, which would cause elements to go to dictionary mode and result
       // in a map change.
       BuildCheckPrototypeMaps(
           handle(JSObject::cast(receiver_map->prototype()), isolate()),
           Handle<JSObject>::null());
 
       // Threshold for fast inlined Array.shift().
       HConstant* inline_threshold = Add<HConstant>(static_cast<int32_t>(16));
 
-      Drop(expr->arguments()->length());
+      Drop(args_count_no_receiver);
       HValue* receiver = Pop();
       HValue* function = Pop();
       HValue* result;
 
       {
         NoObservableSideEffectsScope scope(this);
 
         HValue* length = Add<HLoadNamedField>(
             receiver, static_cast<HValue*>(NULL),
             HObjectAccess::ForArrayLength(kind));
@@ skipping 290 matching lines @@
   HInstruction* call = New<HCallWithDescriptor>(
       code_value, argc + 1, descriptor,
       Vector<HValue*>(op_vals, descriptor->environment_length()));
 
   if (drop_extra) Drop(1);  // Drop function.
   ast_context()->ReturnInstruction(call, ast_id);
   return true;
 }
 
 
-bool HOptimizedGraphBuilder::TryCallApply(Call* expr) {
+void HOptimizedGraphBuilder::HandleIndirectCall(Call* expr,
+                                                HValue* function,
+                                                int arguments_count) {
+  Handle<JSFunction> known_function;
+  int args_count_no_receiver = arguments_count - 1;
+  if (function->IsConstant() &&
+      HConstant::cast(function)->handle(isolate())->IsJSFunction()) {
+    known_function = Handle<JSFunction>::cast(
+        HConstant::cast(function)->handle(isolate()));
+    if (TryInlineApply(known_function, expr, args_count_no_receiver)) {

[Toon Verwaest, 2014/06/12 21:41:13]

Seems like TryIndirectCall splits to apply and call handling, which join again
in HandleIndirectCall. It's a bit weird that it here again has special handling
for Apply. Is it really specific to Apply, or should it just be named
TryInlineIndirectCall?

Or perhaps we can even merge it with TryInlineCall since they don't behave that
different anymore.

[p.antonov, 2014/06/13 08:34:24]

Done.

+      return;
+    }
+
+    Handle<Map> map;
+    HValue* receiver = environment()->ExpressionStackAt(args_count_no_receiver);

[Toon Verwaest, 2014/06/12 21:41:13]

Can you move this code into TryInlineBuiltinMethodCall? 

[p.antonov, 2014/06/13 08:34:24]

Done.

+    if (receiver->IsConstant() &&
+        HConstant::cast(receiver)->handle(isolate())->IsHeapObject()) {
+      map = handle(Handle<HeapObject>::cast(
+          HConstant::cast(receiver)->handle(isolate()))->map());
+    }
+    if (TryInlineBuiltinMethodCall(expr, receiver, map, known_function,

[Toon Verwaest, 2014/06/12 21:41:13]

This was already done in BuildFunctionCallCall. Can we avoid this duplication?
Either pass in a flag for the extra behavior, or move this out to the Apply
variant. Well, I guess we first have to think about the extra behavior tout
court... see the comment down below.

[p.antonov, 2014/06/13 08:34:24]

Done.

+                                   args_count_no_receiver)) {
+      return;
+    }
+  }
+
+  PushArgumentsFromEnvironment(arguments_count);
+  HInvokeFunction* call = New<HInvokeFunction>(
+      function, known_function, arguments_count);
+  Drop(1);  // Function
+  ast_context()->ReturnInstruction(call, expr->id());
+}
+
+
+bool HOptimizedGraphBuilder::TryIndirectCall(Call* expr) {
   ASSERT(expr->expression()->IsProperty());
 
   if (!expr->IsMonomorphic()) {
     return false;
   }
   Handle<Map> function_map = expr->GetReceiverTypes()->first();
   if (function_map->instance_type() != JS_FUNCTION_TYPE ||
-      !expr->target()->shared()->HasBuiltinFunctionId() ||
-      expr->target()->shared()->builtin_function_id() != kFunctionApply) {
+      !expr->target()->shared()->HasBuiltinFunctionId()) {

[Toon Verwaest, 2014/06/12 21:41:13]

What about creating both BuildFunctionCall and BuildFunctionApply, and using a
switch to jump to those? I think you did that in your previous CL... That makes
it a bit more symmetrical.

[p.antonov, 2014/06/13 08:34:24]

Done.

+    return false;
+  }
+  if (expr->target()->shared()->builtin_function_id() == kFunctionCall) {
+    BuildFunctionCallCall(expr);
+    return true;
+  }
+  if (expr->target()->shared()->builtin_function_id() != kFunctionApply) {
     return false;
   }
 
   if (current_info()->scope()->arguments() == NULL) return false;
 
   ZoneList<Expression*>* args = expr->arguments();
   if (args->length() != 2) return false;
 
   VariableProxy* arg_two = args->at(1)->AsVariableProxy();
   if (arg_two == NULL || !arg_two->var()->IsStackAllocated()) return false;
@@ skipping 22 matching lines @@
     ASSERT_EQ(environment()->arguments_environment()->parameter_count(),
               function_state()->entry()->arguments_object()->arguments_count());
     HArgumentsObject* args = function_state()->entry()->arguments_object();
     const ZoneList<HValue*>* arguments_values = args->arguments_values();
     int arguments_count = arguments_values->length();
     Push(function);
     Push(BuildWrapReceiver(receiver, function));
     for (int i = 1; i < arguments_count; i++) {
       Push(arguments_values->at(i));
     }
-
-    Handle<JSFunction> known_function;
-    if (function->IsConstant() &&
-        HConstant::cast(function)->handle(isolate())->IsJSFunction()) {
-      known_function = Handle<JSFunction>::cast(
-          HConstant::cast(function)->handle(isolate()));
-      int args_count = arguments_count - 1;  // Excluding receiver.
-      if (TryInlineApply(known_function, expr, args_count)) return true;
-    }
-
-    PushArgumentsFromEnvironment(arguments_count);
-    HInvokeFunction* call = New<HInvokeFunction>(
-        function, known_function, arguments_count);
-    Drop(1);  // Function.
-    ast_context()->ReturnInstruction(call, expr->id());
+    HandleIndirectCall(expr, function, arguments_count);
     return true;
   }
 }
 
 
+// f.call(...)
+void HOptimizedGraphBuilder::BuildFunctionCallCall(Call* expr) {
+  HValue* function = Pop();  // f
+  HValue* receiver;
+  ZoneList<Expression*>* args = expr->arguments();
+  int args_length = args->length();
+  bool is_builtin_call = false;
+  Handle<JSFunction> known_function;
+  Drop(1);  // call
+
+  if (function->IsConstant() &&
+      HConstant::cast(function)->handle(isolate())->IsJSFunction()) {
+    known_function = Handle<JSFunction>::cast(
+          HConstant::cast(function)->handle(isolate()));
+    is_builtin_call = known_function->shared()->HasBuiltinFunctionId();
+  }
+
+  ArgumentsAllowedFlag flag = is_builtin_call
+      ? ARGUMENTS_ALLOWED : ARGUMENTS_NOT_ALLOWED;
+
+  if (args_length == 0) {
+    receiver = graph()->GetConstantUndefined();
+    args_length = 1;
+  } else {
+    CHECK_ALIVE(VisitForValue(args->at(0), flag));
+    receiver = Pop();
+  }
+  receiver = is_builtin_call ? receiver : BuildWrapReceiver(receiver, function);

[Toon Verwaest, 2014/06/12 21:41:14]

You can't just skip BuildWrapReceiver for builtins. BuildWrapReceiver has the
required logic to figure out whether it can be skipped or not (skipped only when
it's in strict-mode). Slice is in strict-mode, which is what you care about
most, so that should work out I presume?

[p.antonov, 2014/06/13 08:34:24]

Done.

+
+  Push(function);
+  Push(receiver);
+  for (int i = 1; i < args_length; i++) {
+    CHECK_ALIVE(VisitForValue(args->at(i), flag));
+  }
+
+  if (is_builtin_call) {
+    ASSERT(!known_function.is_null());
+
+    Handle<Map> map;
+    if (receiver->IsConstant() &&
+        HConstant::cast(receiver)->handle(isolate())->IsHeapObject()) {
+      map = handle(Handle<HeapObject>::cast(
+          HConstant::cast(receiver)->handle(isolate()))->map());
+    }
+
+    int args_count_no_receiver = args_length - 1;
+    if (TryInlineBuiltinMethodCall(expr, receiver, map, known_function,
+                                   args_count_no_receiver)) {
+      return;
+    }
+    // Failing to inline as a builtin means arguments cannot
+    // be allowed after all.

[Toon Verwaest, 2014/06/12 21:41:13]

Iek! So what if I do the following?

a = [];
a.push(arguments);
return a;

[p.antonov, 2014/06/13 08:34:24]

Done.

+    for (int i = 0; i < args_length; ++i) {
+      HValue* arg = environment()->ExpressionStackAt(i);
+      if (arg->CheckFlag(HValue::kIsArguments)) {
+        return Bailout(kBadValueContextForArgumentsValue);
+      }
+    }
+  }
+  HandleIndirectCall(expr, function, args_length);
+}
+
+
 HValue* HOptimizedGraphBuilder::ImplicitReceiverFor(HValue* function,
                                                     Handle<JSFunction> target) {
   SharedFunctionInfo* shared = target->shared();
   if (shared->strict_mode() == SLOPPY && !shared->native()) {
     // Cannot embed a direct reference to the global proxy
     // as is it dropped on deserialization.
     CHECK(!isolate()->serializer_enabled());
     Handle<JSObject> global_receiver(
         target->context()->global_object()->global_receiver());
     return Add<HConstant>(global_receiver);
@@ skipping 241 matching lines @@
     environment()->SetExpressionStackAt(0, function);
 
     Push(receiver);
 
     if (function->IsConstant() &&
         HConstant::cast(function)->handle(isolate())->IsJSFunction()) {
       Handle<JSFunction> known_function = Handle<JSFunction>::cast(
           HConstant::cast(function)->handle(isolate()));
       expr->set_target(known_function);
 
-      if (TryCallApply(expr)) return;
+      if (TryIndirectCall(expr)) return;
       CHECK_ALIVE(VisitExpressions(expr->arguments()));
 
       Handle<Map> map = types->length() == 1 ? types->first() : Handle<Map>();
-      if (TryInlineBuiltinMethodCall(expr, receiver, map)) {
-        if (FLAG_trace_inlining) {
-          PrintF("Inlining builtin ");
-          known_function->ShortPrint();
-          PrintF("\n");
-        }
+      if (TryInlineBuiltinMethodCall(expr, receiver, map, known_function,
+                                     expr->arguments()->length())) {
         return;
       }
       if (TryInlineApiMethodCall(expr, receiver, types)) return;
 
       // Wrap the receiver if necessary.
       if (NeedsWrappingFor(ToType(types->first()), known_function)) {
         // Since HWrapReceiver currently cannot actually wrap numbers and
         // strings, use the regular CallFunctionStub for method calls to wrap
         // the receiver.
         // TODO(verwaest): Support creation of value wrappers directly in
@@ skipping 3454 matching lines @@
   if (ShouldProduceTraceOutput()) {
     isolate()->GetHTracer()->TraceHydrogen(name(), graph_);
   }
 
 #ifdef DEBUG
   graph_->Verify(false);  // No full verify.
 #endif
 }
 
 } }  // namespace v8::internal