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

runtime/vm/precompiler.cc

Index: runtime/vm/precompiler.cc
diff --git a/runtime/vm/precompiler.cc b/runtime/vm/precompiler.cc
index ce07486c707488878bde0cd3312493e488848a53..9f8cbca6f794e9939def5c4e67c1a55388c9f351 100644
--- a/runtime/vm/precompiler.cc
+++ b/runtime/vm/precompiler.cc
@@ -830,6 +830,7 @@ void Precompiler::AddCalleesOf(const Function& function) {
 #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);
@@ -838,7 +839,7 @@ void Precompiler::AddCalleesOf(const Function& function) {
   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.
@@ -1032,6 +1033,9 @@ void Precompiler::AddConstObject(const Instance& instance) {
   // 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) {
@@ -1077,6 +1081,8 @@ void Precompiler::AddClosureCall(const Array& arguments_descriptor) {
 
 
 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()) {
@@ -2095,6 +2101,7 @@ void Precompiler::SwitchICCalls() {
           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)),
@@ -2109,8 +2116,10 @@ void Precompiler::SwitchICCalls() {
 
       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
@@ -2150,6 +2159,7 @@ void Precompiler::SwitchICCalls() {
     Code& code_;
     ObjectPool& pool_;
     Object& entry_;
+    TypedData& info_array_;
     ICData& ic_;
     String& target_name_;
     Array& args_descriptor_;