Introduce simple Thread class to support gradual refactoring of interfaces.

runtime/vm/flow_graph.cc

Index: runtime/vm/flow_graph.cc
===================================================================
--- runtime/vm/flow_graph.cc	(revision 43047)
+++ runtime/vm/flow_graph.cc	(working copy)
@@ -22,7 +22,7 @@
 FlowGraph::FlowGraph(ParsedFunction* parsed_function,
                      GraphEntryInstr* graph_entry,
                      intptr_t max_block_id)
-  : isolate_(Isolate::Current()),
+  : thread_(Thread::Current()),
     parent_(),
     current_ssa_temp_index_(0),
     max_block_id_(max_block_id),
@@ -43,8 +43,7 @@
     loop_invariant_loads_(NULL),
     guarded_fields_(parsed_function->guarded_fields()),
     deferred_prefixes_(parsed_function->deferred_prefixes()),
-    captured_parameters_(
-        new(isolate_) BitVector(isolate_, variable_count())) {
+    captured_parameters_(new(zone()) BitVector(zone(), variable_count())) {
   DiscoverBlocks();
 }
 
@@ -98,7 +97,7 @@
   ConstantInstr* constant = constant_instr_pool_.Lookup(object);
   if (constant == NULL) {
     // Otherwise, allocate and add it to the pool.
-    constant = new(isolate()) ConstantInstr(
+    constant = new(zone()) ConstantInstr(
         Object::ZoneHandle(isolate(), object.raw()));
     constant->set_ssa_temp_index(alloc_ssa_temp_index());
 
@@ -224,7 +223,7 @@
 
 void FlowGraph::MergeBlocks() {
   bool changed = false;
-  BitVector* merged = new(isolate()) BitVector(isolate(), postorder().length());
+  BitVector* merged = new(zone()) BitVector(zone(), postorder().length());
   for (BlockIterator block_it = reverse_postorder_iterator();
        !block_it.Done();
        block_it.Advance()) {
@@ -372,7 +371,7 @@
 LivenessAnalysis::LivenessAnalysis(
   intptr_t variable_count,
   const GrowableArray<BlockEntryInstr*>& postorder)
-    : isolate_(Isolate::Current()),
+    : zone_(Zone::Current()),
       variable_count_(variable_count),
       postorder_(postorder),
       live_out_(postorder.length()),
@@ -429,9 +428,9 @@
 void LivenessAnalysis::Analyze() {
   const intptr_t block_count = postorder_.length();
   for (intptr_t i = 0; i < block_count; i++) {
-    live_out_.Add(new(isolate()) BitVector(isolate(), variable_count_));
-    kill_.Add(new(isolate()) BitVector(isolate(), variable_count_));
-    live_in_.Add(new(isolate()) BitVector(isolate(), variable_count_));
+    live_out_.Add(new(zone()) BitVector(zone(), variable_count_));

[siva, 2015/01/21 22:14:50]

This kind of seems fragile to me, what if another Zone scope was started between
the time the constructor for LivenessAnalysis was called
and the call to LivenessAnalysis::Analyze.
The allocations here would then be happening not in the current zone but some
zone above.

[koda, 2015/01/21 23:05:44]

Operator new ASSERTs that the zone is actually still the current zone, so this
is only an issue if the zone is conditionally created, and would it show up in
debug mode.

The same basic issue exists with all kinds of caching. Theoretically, someone
could do SetCurrent to change the isolate between the constructor and Analyze.

[siva, 2015/01/22 01:18:20]

I agree that calling SetCurrent would have the same effect, that seems like an
egregious change if somebody called SetCurrent in the compiler but starting a
new Zone seems like an innocuous change with a nasty side effect.

Do you think we should have a NONEWZONESCOPE or something like that and use that
around code that caches the zone pointer (e.g constructor of LivenessAnalysis
here).

[Ivan Posva, 2015/01/22 01:26:44]

Actually I think the way it is now safer than it was before. With this change
the LivenessAnalysis will have all of its data co-located in the same zone. In
my opinion the NONEWZONESCOPE would be overdoing it and probably be
counterproductive.

[koda, 2015/01/22 02:01:00]

The worst case scenario with a cached zone is that:
1. A new zone is introduced conditionally, but the code inside that scope still
uses the older cached zone.
2. For some reason, this never triggers in debug testing (if it does, the ASSERT
will notify you of the issue).
3. The result is that in release mode, some memory is kept alive longer than the
programmer intended.

By comparison, if instead caching the isolate, introducing another zone could
cause allocations to be freed *earlier* than intended.

+    kill_.Add(new(zone()) BitVector(zone(), variable_count_));
+    live_in_.Add(new(zone()) BitVector(zone(), variable_count_));
   }
 
   ComputeInitialSets();
@@ -548,7 +547,7 @@
 void VariableLivenessAnalysis::ComputeInitialSets() {
   const intptr_t block_count = postorder_.length();
 
-  BitVector* last_loads = new(isolate()) BitVector(isolate(), variable_count_);
+  BitVector* last_loads = new(zone()) BitVector(zone(), variable_count_);
   for (intptr_t i = 0; i < block_count; i++) {
     BlockEntryInstr* block = postorder_[i];
 
@@ -674,7 +673,7 @@
     idom.Add(parent_[i]);
     semi.Add(i);
     label.Add(i);
-    dominance_frontier->Add(new(isolate()) BitVector(isolate(), size));
+    dominance_frontier->Add(new(zone()) BitVector(zone(), size));
   }
 
   // Loop over the blocks in reverse preorder (not including the graph
@@ -844,7 +843,7 @@
     // are unknown and so treated like parameters.
     intptr_t count = IsCompiledForOsr() ? variable_count() : parameter_count();
     for (intptr_t i = 0; i < count; ++i) {
-      ParameterInstr* param = new(isolate()) ParameterInstr(i, entry);
+      ParameterInstr* param = new(zone()) ParameterInstr(i, entry);
       param->set_ssa_temp_index(alloc_ssa_temp_index());  // New SSA temp.
       AddToInitialDefinitions(param);
       env.Add(param);
@@ -930,7 +929,7 @@
   } else if (block_entry->IsCatchBlockEntry()) {
     // Add real definitions for all locals and parameters.
     for (intptr_t i = 0; i < env->length(); ++i) {
-      ParameterInstr* param = new(isolate()) ParameterInstr(i, block_entry);
+      ParameterInstr* param = new(zone()) ParameterInstr(i, block_entry);
       param->set_ssa_temp_index(alloc_ssa_temp_index());  // New SSA temp.
       (*env)[i] = param;
       block_entry->AsCatchBlockEntry()->initial_definitions()->Add(param);
@@ -1109,7 +1108,7 @@
         PhiInstr* phi = (*successor->phis())[i];
         if (phi != NULL) {
           // Rename input operand.
-          Value* use = new(isolate()) Value((*env)[i]);
+          Value* use = new(zone()) Value((*env)[i]);
           phi->SetInputAt(pred_index, use);
         }
       }
@@ -1194,7 +1193,7 @@
 // Design & Implementation" (Muchnick) p192.
 BitVector* FlowGraph::FindLoop(BlockEntryInstr* m, BlockEntryInstr* n) const {
   GrowableArray<BlockEntryInstr*> stack;
-  BitVector* loop = new(isolate()) BitVector(isolate(), preorder_.length());
+  BitVector* loop = new(zone()) BitVector(zone(), preorder_.length());
 
   loop->Add(n->preorder_number());
   if (n != m) {
@@ -1218,7 +1217,7 @@
 
 ZoneGrowableArray<BlockEntryInstr*>* FlowGraph::ComputeLoops() const {
   ZoneGrowableArray<BlockEntryInstr*>* loop_headers =
-      new(isolate()) ZoneGrowableArray<BlockEntryInstr*>();
+      new(zone()) ZoneGrowableArray<BlockEntryInstr*>();
 
   for (BlockIterator it = postorder_iterator();
        !it.Done();
@@ -1279,7 +1278,7 @@
 
 
 void FlowGraph::ComputeBlockEffects() {
-  block_effects_ = new(isolate()) BlockEffects(this);
+  block_effects_ = new(zone()) BlockEffects(this);
 }
 
 
@@ -1287,11 +1286,11 @@
     : available_at_(flow_graph->postorder().length()) {
   // We are tracking a single effect.
   ASSERT(EffectSet::kLastEffect == 1);
-  Isolate* isolate = flow_graph->isolate();
+  Zone* zone = flow_graph->zone();
   const intptr_t block_count = flow_graph->postorder().length();
 
   // Set of blocks that contain side-effects.
-  BitVector* kill = new(isolate) BitVector(isolate, block_count);
+  BitVector* kill = new(zone) BitVector(zone, block_count);
 
   // Per block available-after sets. Block A is available after the block B if
   // and only if A is either equal to B or A is available at B and B contains no
@@ -1317,7 +1316,7 @@
     }
   }
 
-  BitVector* temp = new(isolate) BitVector(isolate, block_count);
+  BitVector* temp = new(zone) BitVector(zone, block_count);
 
   // Recompute available-at based on predecessors' available-after until the fix
   // point is reached.
@@ -1350,9 +1349,9 @@
         // Available-at changed: update it and recompute available-after.
         if (available_at_[block_num] == NULL) {
           current = available_at_[block_num] =
-              new(isolate) BitVector(isolate, block_count);
+              new(zone) BitVector(zone, block_count);
           available_after[block_num] =
-              new(isolate) BitVector(isolate, block_count);
+              new(zone) BitVector(zone, block_count);
           // Block is always available after itself.
           available_after[block_num]->Add(block_num);
         }