Cache lookups at megamorphic call sites in optimized code.

runtime/vm/code_generator.cc

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/code_generator.h"
 
 #include "vm/assembler_macros.h"
 #include "vm/ast.h"
 #include "vm/bigint_operations.h"
 #include "vm/code_patcher.h"
@@ skipping 993 matching lines @@
   GrowableArray<const Instance*> args(3);
   args.Add(&receiver);
   args.Add(&arg1);
   args.Add(&arg2);
   const Function& result =
       Function::Handle(InlineCacheMissHandler(isolate, args));
   arguments.SetReturn(result);
 }
 
 
+//   Arg0: Receiver's class ID.

[srdjan, 2012/12/03 19:10:46]

This seems to be receiver not cid. Arg1 is ICData ... please fix the rest of the
comment.

[Kevin Millikin (Google), 2012/12/06 14:03:11]

Comment is fixed.

+//   Arg1: Megamorphic cache.
+//   Arg2: Arguments descriptor.
+//   Arg3: IC data.

[srdjan, 2012/12/03 19:10:46]

Describe results (target not found vs found).

+DEFINE_RUNTIME_ENTRY(MegamorphicCacheMissHandler, 3) {
+  ASSERT(arguments.ArgCount() ==
+     kMegamorphicCacheMissHandlerRuntimeEntry.argument_count());
+  const Instance& receiver =
+      Instance::CheckedHandle(isolate, arguments.ArgAt(0));
+  const ICData& ic_data = ICData::CheckedHandle(isolate, arguments.ArgAt(1));
+  const Array& descriptor = Array::CheckedHandle(isolate, arguments.ArgAt(2));
+  const String& name = String::Handle(isolate, ic_data.target_name());
+  const MegamorphicCache& cache = MegamorphicCache::Handle(isolate,
+      isolate->megamorphic_cache_table()->Lookup(name, descriptor));
+  Class& cls = Class::Handle(isolate, receiver.clazz());
+  if (cls.IsNullClass()) {

[Vyacheslav Egorov (Google), 2012/12/03 14:55:22]

Please add a comment.

[Kevin Millikin (Google), 2012/12/06 14:03:11]

The comment at the site I took this from said "For lookups treat null as an
instance of class Object."

I have copied it here, but I don't think it's a very good comment.  It just
repeats exactly what the code says, and doesn't have any indication *why* it
should work.  For instance, I'm somewhat surprised that we want to be invoking
Object::hashCode on null.

+    cls = isolate->object_store()->object_class();
+  }
+  ASSERT(!cls.IsNull());
+  intptr_t arg_count =
+      Smi::Cast(Object::Handle(isolate, descriptor.At(0))).Value();

[Vyacheslav Egorov (Google), 2012/12/03 14:55:22]

I know that this is not your code but I really don't like that currently we are
spreading magic constants all the way through our code when working with
arguments descriptor. 

descriptor.At(0) does not communicate intent correctly.

I would prefer to have a wrapper similar to Code::Comments that encapsulates
this knowledge.

[Kevin Millikin (Google), 2012/12/06 14:03:11]

I have fixed this in another change.

+  intptr_t named_arg_count =
+      arg_count - Smi::Cast(Object::Handle(isolate, descriptor.At(1))).Value();
+  const Function& target = Function::Handle(isolate,
+      Resolver::ResolveDynamicForReceiverClass(cls,
+                                               name,
+                                               arg_count,
+                                               named_arg_count));
+
+  Instructions& instructions = Instructions::Handle(isolate);
+  if (!target.IsNull()) {
+    if (!target.HasCode()) {
+      const Error& error =
+          Error::Handle(isolate, Compiler::CompileFunction(target));
+      if (!error.IsNull()) Exceptions::PropagateError(error);
+    }
+    ASSERT(target.HasCode());
+    instructions = Code::Handle(isolate, target.CurrentCode()).instructions();
+  }
+  arguments.SetReturn(instructions);
+  if (instructions.IsNull()) return;

[srdjan, 2012/12/03 19:10:46]

Would the code above be simpler if changed to:

arguments.SetReturn(Instruction::Handle());
if (target.IsNull()) {
  return;
}

if (!target.HasCode()) {
  // compile, throw error and return if necessary
}

const Instructions& instructions = ....

[Kevin Millikin (Google), 2012/12/06 14:03:11]

Possibly.  I think we can sort out the code duplication here and in
ResolveCompileInstanceCallTarget (used by the polymorphic ICs), but I want to
leave that for a separate change.

+
+  cache.EnsureCapacity();
+  const Smi& class_id = Smi::Handle(isolate, Smi::New(cls.id()));
+  cache.Insert(class_id, target);
+  return;

[srdjan, 2012/12/03 19:10:46]

I also think that using isolate here explicitly makes the code look clumsier,
without any performance impact.

[Kevin Millikin (Google), 2012/12/06 14:03:11]

Removed.

+}

[srdjan, 2012/12/03 19:10:46]

Could you also add tracing flag and code to be able to monitor the megamorphic
misses.

[Kevin Millikin (Google), 2012/12/06 14:03:11]

I just printed something for --trace-ic and --trace-ic-miss-in-optimized.  Since
I haven't used those flags, I'm not sure if I'm printing the most useful
possible information.

+
+
 // Updates IC data for two arguments. Used by the equality operation when
 // the control flow bypasses regular inline cache (null arguments).
 //   Arg0: Receiver object.
 //   Arg1: Argument after receiver.
 //   Arg2: Target's name.
 //   Arg3: ICData.
 DEFINE_RUNTIME_ENTRY(UpdateICDataTwoArgs, 4) {
   ASSERT(arguments.ArgCount() ==
       kUpdateICDataTwoArgsRuntimeEntry.argument_count());
   const Instance& receiver = Instance::CheckedHandle(arguments.ArgAt(0));
@@ skipping 855 matching lines @@
   Isolate* isolate = Isolate::Current();
   StackZone zone(isolate);
   HANDLESCOPE(isolate);
   const Bigint& big_left = Bigint::Handle(left);
   const Bigint& big_right = Bigint::Handle(right);
   return BigintOperations::Compare(big_left, big_right);
 }
 END_LEAF_RUNTIME_ENTRY
 
 }  // namespace dart