VM: Make precompiler process constants/fields only once, avoid using pool.InfoAt()

runtime/vm/precompiler.cc

 // Copyright (c) 2015, 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/precompiler.h"
 
 #include "vm/aot_optimizer.h"
 #include "vm/assembler.h"
 #include "vm/ast_printer.h"
 #include "vm/branch_optimizer.h"
@@ skipping 812 matching lines @@
       target ^= entry.raw();
       AddFunction(target);
     }
   }
 
 #if defined(TARGET_ARCH_IA32)
   FATAL("Callee scanning unimplemented for IA32");
 #endif
 
   const ObjectPool& pool = ObjectPool::Handle(Z, code.GetObjectPool());
+  ObjectPoolInfo pool_info(pool);
   ICData& call_site = ICData::Handle(Z);
   MegamorphicCache& cache = MegamorphicCache::Handle(Z);
   String& selector = String::Handle(Z);
   Field& field = Field::Handle(Z);
   Class& cls = Class::Handle(Z);
   Instance& instance = Instance::Handle(Z);
   Code& target_code = Code::Handle(Z);
   for (intptr_t i = 0; i < pool.Length(); i++) {
-    if (pool.InfoAt(i) == ObjectPool::kTaggedObject) {
+    if (pool_info.InfoAt(i) == ObjectPool::kTaggedObject) {

[kustermann, 2016/12/08 13:52:44]

This moves the handle allocation out of the loop.

Side note: Note that all these handles here which are not used/necessary after
the call to AddCalleesOf() is done, will be kept alive for a long time: It seems
the next closest handle scope upwards the stack is the global one from here:


 323 void Precompiler::DoCompileAll(
 324     Dart_QualifiedFunctionName embedder_entry_points[]) {
 325   ASSERT(I->compilation_allowed());
 326 
 327   {
 328     StackZone stack_zone(T);
 329     zone_ = stack_zone.GetZone();
 330     
 331     { 
 332       HANDLESCOPE(T);

       entry = pool.ObjectAt(i);
       if (entry.IsICData()) {
         // A dynamic call.
         call_site ^= entry.raw();
         ASSERT(!call_site.is_static_call());
         selector = call_site.target_name();
         AddSelector(selector);
         if (selector.raw() == Symbols::Call().raw()) {
           // Potential closure call.
           const Array& arguments_descriptor =
@@ skipping 173 matching lines @@
     return;
   }
 
   // Can't ask immediate objects if they're canoncial.
   if (instance.IsSmi()) return;
 
   // Some Instances in the ObjectPool aren't const objects, such as
   // argument descriptors.
   if (!instance.IsCanonical()) return;
 
+  // Constants are canonicalized and we avoid repeated processing of them.
+  if (consts_to_retain_.Lookup(&instance) != NULL) return;
+
   consts_to_retain_.Insert(&Instance::ZoneHandle(Z, instance.raw()));

[kustermann, 2016/12/08 13:52:44]

Please note that

  consts_to_retain_.Insert()

calls

  DirectChainedHashMap<InstanceKeyValueTrait>::Insert()

which seems to unconditionally add new elements (even if they already exist)
See here:
https://github.com/dart-lang/sdk/tree/fa038d003baa411f2f39d70c73eb745b9707fc5...


As a side note: This DirectChainedHashMap is really weired (one reason is the
Insert() behavior), since:

  map.Insert(Pair(mykey, 1));
  map.Insert(Pair(mykey, 2));
  map.Lookup(mykey);

might give you 1 back or 2 -- depends on rehashing AFAIK!

Luckily in this case we treat it as a Set so it makes no difference (except for
increased memory footprint for duplicate entries).

[Vyacheslav Egorov (Google), 2016/12/08 18:18:19]

What about adding an ASSERT there?

[kustermann, 2016/12/08 19:14:38]

If it's fine with you, I'll do it in a different CL

There might be other places where we accidentally Insert() the same key twice.
So after adding the ASSERT() I should test this in many configurations which
takes a long time.

 
   if (cls.NumTypeArguments() > 0) {
     AddTypeArguments(TypeArguments::Handle(Z, instance.GetTypeArguments()));
   }
 
   class ConstObjectVisitor : public ObjectPointerVisitor {
    public:
     ConstObjectVisitor(Precompiler* precompiler, Isolate* isolate)
         : ObjectPointerVisitor(isolate),
           precompiler_(precompiler),
@@ skipping 24 matching lines @@
       Class::Handle(Z, I->object_store()->closure_class());
   const Function& dispatcher = Function::Handle(
       Z, cache_class.GetInvocationDispatcher(
              Symbols::Call(), arguments_descriptor,
              RawFunction::kInvokeFieldDispatcher, true /* create_if_absent */));
   AddFunction(dispatcher);
 }
 
 
 void Precompiler::AddField(const Field& field) {
+  if (fields_to_retain_.Lookup(&field) != NULL) return;
+
   fields_to_retain_.Insert(&Field::ZoneHandle(Z, field.raw()));
 
   if (field.is_static()) {
     const Object& value = Object::Handle(Z, field.StaticValue());
     if (value.IsInstance()) {
       AddConstObject(Instance::Cast(value));
     }
 
     if (field.has_initializer()) {
       // Should not be in the middle of initialization while precompiling.
@@ skipping 998 matching lines @@
   // (cid, target function, count) to (cid, target code, entry point), and
   // replace the ICCallThroughFunction stub with ICCallThroughCode.
 
   class SwitchICCallsVisitor : public FunctionVisitor {
    public:
     explicit SwitchICCallsVisitor(Zone* zone)
         : zone_(zone),
           code_(Code::Handle(zone)),
           pool_(ObjectPool::Handle(zone)),
           entry_(Object::Handle(zone)),
+          info_array_(TypedData::Handle(zone)),
           ic_(ICData::Handle(zone)),
           target_name_(String::Handle(zone)),
           args_descriptor_(Array::Handle(zone)),
           unlinked_(UnlinkedCall::Handle(zone)),
           target_code_(Code::Handle(zone)),
           canonical_unlinked_calls_() {}
 
     void Visit(const Function& function) {
       if (!function.HasCode()) {
         return;
       }
 
       code_ = function.CurrentCode();
       pool_ = code_.object_pool();
+      info_array_ = pool_.info_array();
+      ObjectPoolInfo pool_info(info_array_);

[Florian Schneider, 2016/12/08 18:30:45]

Why not just:

ObjectPoolInfo pool_info(pool_);

[kustermann, 2016/12/08 19:14:38]

So we allocate one handle per [SwitchICCallsVisitor] instead of one new handle
for every single [Function] it visits.

(i.e. Same reason why all the other handles are instance variables instead of
method locals.)

       for (intptr_t i = 0; i < pool_.Length(); i++) {
-        if (pool_.InfoAt(i) != ObjectPool::kTaggedObject) continue;
+        if (pool_info.InfoAt(i) != ObjectPool::kTaggedObject) continue;

[kustermann, 2016/12/08 13:52:44]

Same thing: Move handle allocation out of loop.

         entry_ = pool_.ObjectAt(i);
         if (entry_.IsICData()) {
           // The only IC calls generated by precompilation are for switchable
           // calls.
           ic_ ^= entry_.raw();
           ic_.ResetSwitchable(zone_);
 
           unlinked_ = UnlinkedCall::New();
           target_name_ = ic_.target_name();
           unlinked_.set_target_name(target_name_);
@@ skipping 19 matching lines @@
       } else {
         return canonical_unlinked->raw();
       }
     }
 
    private:
     Zone* zone_;
     Code& code_;
     ObjectPool& pool_;
     Object& entry_;
+    TypedData& info_array_;
     ICData& ic_;
     String& target_name_;
     Array& args_descriptor_;
     UnlinkedCall& unlinked_;
     Code& target_code_;
     UnlinkedCallSet canonical_unlinked_calls_;
   };
 
   ASSERT(!I->compilation_allowed());
   SwitchICCallsVisitor visitor(Z);
@@ skipping 1125 matching lines @@
 
   ASSERT(FLAG_precompiled_mode);
   const bool optimized = function.IsOptimizable();  // False for natives.
   DartPrecompilationPipeline pipeline(zone, field_type_map);
   return PrecompileFunctionHelper(precompiler, &pipeline, function, optimized);
 }
 
 #endif  // DART_PRECOMPILER
 
 }  // namespace dart