Precompilation: compact the symbol table.

runtime/vm/symbols.cc

Index: runtime/vm/symbols.cc
diff --git a/runtime/vm/symbols.cc b/runtime/vm/symbols.cc
index e984c1bd16bc2fa2b8d5bbb1583791c8567f4f83..00f9571c43f839599baf03869f800bac913f26f3 100644
--- a/runtime/vm/symbols.cc
+++ b/runtime/vm/symbols.cc
@@ -331,6 +331,90 @@ void Symbols::SetupSymbolTable(Isolate* isolate) {
 }
 
 
+intptr_t Symbols::Compact(Isolate* isolate) {
+  Zone* zone = Thread::Current()->zone();
+  intptr_t initial_size = -1;
+  intptr_t final_size = -1;
+
+  // 1. Build a collection of all the predefined symbols so they are
+  // strongly referenced (the read only handles are not traced).
+  GrowableObjectArray& predefined_symbols =
+      GrowableObjectArray::Handle(GrowableObjectArray::New());

[siva, 2016/01/28 00:04:00]

Can this be moved inside the scope of
if (Object::vm_isolate_sna...... == 0) {
as
GrowableObjectArray& predefined_symbols =
 
GrowableObjectArray::Handle(GrowableObjectArray::New(Symbols::kMaxPredefinedId));

[rmacnak, 2016/01/28 17:53:09]

Done.

+  {
+    String& symbol = String::Handle();

[siva, 2016/01/28 00:03:59]

This handle could also go inside the scope of
if (Object::vm_isolate..... == 0) {

[rmacnak, 2016/01/28 17:53:09]

Done.

+    SymbolTable table(zone, isolate->object_store()->symbol_table());
+    initial_size = table.NumOccupied();
+
+    if (Object::vm_isolate_snapshot_object_table().Length() == 0) {
+      for (intptr_t i = 1; i < Symbols::kNullCharId; i++) {
+        const unsigned char* name =
+          reinterpret_cast<const unsigned char*>(names[i]);
+        symbol ^= table.GetOrNull(Latin1Array(name, strlen(names[i])));
+        ASSERT(!symbol.IsNull());
+        predefined_symbols.Add(symbol);
+      }
+      for (intptr_t c = 0; c < kNumberOfOneCharCodeSymbols; c++) {
+        intptr_t idx = (kNullCharId + c);
+        ASSERT(idx < kMaxPredefinedId);
+        ASSERT(Utf::IsLatin1(c));
+        uint8_t ch = static_cast<uint8_t>(c);
+        symbol ^= table.GetOrNull(Latin1Array(&ch, 1));
+        ASSERT(!symbol.IsNull());
+        predefined_symbols.Add(symbol);
+      }
+    }
+    table.Release();
+  }
+
+  // 2. Knock out the symbol table and do a full garbage collection.
+  isolate->object_store()->set_symbol_table(Object::empty_array());
+  isolate->heap()->CollectAllGarbage();
+
+  // 3. Walk the heap and built a new table from surviving symbols.

[siva, 2016/01/27 21:21:10]

Walk the heap and build a new 

[rmacnak, 2016/01/27 23:06:20]

Done.

+  GrowableArray<String*> symbols;
+  class SymbolCollector : public ObjectVisitor {
+   public:
+    SymbolCollector(GrowableArray<String*>* symbols,
+                    Isolate* isolate,
+                    Zone* zone)
+        : ObjectVisitor(isolate),
+          symbols_(symbols),
+          zone_(zone) {}

[siva, 2016/01/28 00:04:00]

Why not make the signature
SymbolCollector(Thread* thread, &symbols)
  : ObjectVisitor(thread->isolate()),
    symbols_(symbols),
    zone_(thread->zone()) {}

to be consistent with other signatures.

[rmacnak, 2016/01/28 17:53:09]

Done.

+
+    void VisitObject(RawObject* obj) {
+      if (obj->IsString() && obj->IsCanonical()) {

[siva, 2016/01/28 00:04:00]

would if (obj->IsCanonical() && obj->IsString()) be more optimal?

[rmacnak, 2016/01/28 17:53:09]

Probably a little bit since canonical is a bit check and string is a range
check.

+        symbols_->Add(&String::ZoneHandle(zone_, String::RawCast(obj)));
+      }
+    }
+
+   private:
+    GrowableArray<String*>* symbols_;
+    Zone* zone_;
+  };
+
+  SymbolCollector visitor(&symbols, isolate, zone);
+  isolate->heap()->IterateObjects(&visitor);
+
+  {
+    Array& array =
+        Array::Handle(HashTables::New<SymbolTable>(kInitialSymtabSize,

[siva, 2016/01/27 21:21:10]

Instead of kInitialSymtabSize you could count the number of symbols seen in the
iterator and use that (+ number of Predefined symbols, see comment below).

[rmacnak, 2016/01/27 23:06:20]

Done. (plus load factor)

+                                                   Heap::kOld));
+    SymbolTable table(zone, array.raw());
+    for (intptr_t i = 0; i < symbols.length(); i++) {
+      String& symbol = *symbols[i];
+      ASSERT(symbol.IsString());
+      ASSERT(symbol.IsCanonical());
+      bool present = table.Insert(symbol);
+      ASSERT(!present);
+    }
+    final_size = table.NumOccupied();
+    isolate->object_store()->set_symbol_table(table.Release());

[siva, 2016/01/27 21:21:10]

Instead of creating a strong reference to the predefined symbols above why not
just call Symbols::AddPredefinedSymbolsToIsolate() here so that they get added
to the new symbol table.

[rmacnak, 2016/01/27 23:06:20]

As discussed, would leave some read-only handles pointing to garbage.

+  }
+
+  return initial_size - final_size;
+}
+
+
 void Symbols::GetStats(Isolate* isolate, intptr_t* size, intptr_t* capacity) {
   ASSERT(isolate != NULL);
   SymbolTable table(isolate->object_store()->symbol_table());