Thread/Isolate refactoring: new(Isolate) -> new(Zone)

Thread/Isolate refactoring: new(Isolate*) -> new(Zone*)

Refactor all remaning cases where the current zone is used through new(Isolate*) and remove this interface.

Removing this interface is needed to move towards multiple threads per isolate, and also makes the caller more aware of the scope of the zone used, reducing the risk of use-after-free.

Make the current thread and the stack zone created around native/runtime entries directly available in their body, saving an indirection (and optimized away if unused).

R=iposva@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44541

[koda, 2015-03-06 15:12:04]

koda@google.com changed reviewers:
+ iposva@google.com

[koda, 2015-03-06 21:05:43]

The next step will be to remove the remaining uses of Isolate::current_zone()
and pivot Isolate* -> Thread* for HANDLESCOPE, etc.

[Ivan Posva, 2015-03-13 21:56:50]

First round of comments.
-Ivan

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:83: Zone* zone) = 0;
We generally pass Isolate* as the first parameter, probably should do the same
for Zone*.

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:155: Zone* zone_;
Where is zone_ being set?

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:139: if (env != NULL) env->DeepCopyTo(zone(), instr);
Please add
{
  ...
}

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:152: if (env != NULL) env->DeepCopyTo(zone(), instr);
ditto

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/regexp.cc#new...
runtime/vm/regexp.cc:16: #define CZ (compiler->zone())
I know this is not your code, but please investigate the difference between Z
and CZ, which I find very surprising. Are they pointing to the same zone and
just use different accessors?

[koda, 2015-03-14 00:46:37]

iposva: Addressed comments and updated CZ macro; PTAL

zerny: Please verify that the CZ->OZ change fits with the intended design (I've
verified that it currently has no effects).

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/compiler.cc
File runtime/vm/compiler.cc (right):

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:83: Zone* zone) = 0;
On 2015/03/13 21:56:49, Ivan Posva wrote:
> We generally pass Isolate* as the first parameter, probably should do the same
> for Zone*.

Done.

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/compiler.cc#n...
runtime/vm/compiler.cc:155: Zone* zone_;
On 2015/03/13 21:56:49, Ivan Posva wrote:
> Where is zone_ being set?

Removed (leftover from previous version).

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/flow_graph.cc
File runtime/vm/flow_graph.cc (right):

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:139: if (env != NULL) env->DeepCopyTo(zone(), instr);
On 2015/03/13 21:56:50, Ivan Posva wrote:
> Please add
> {
>   ...
> }

Done.

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:152: if (env != NULL) env->DeepCopyTo(zone(), instr);
On 2015/03/13 21:56:50, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/regexp.cc
File runtime/vm/regexp.cc (right):

https://codereview.chromium.org/982873004/diff/60001/runtime/vm/regexp.cc#new...
runtime/vm/regexp.cc:16: #define CZ (compiler->zone())
On 2015/03/13 21:56:50, Ivan Posva wrote:
> I know this is not your code, but please investigate the difference between Z
> and CZ, which I find very surprising. Are they pointing to the same zone and
> just use different accessors?

Within this file, Z and CZ always refer to the same zone, which is also the
topmost zone of the thread. (I've verified this manually, but not sure we'd want
to ASSERT this in DEBUG mode, since it is not necessarily needed by the design.)

CZ is used in the visitor-pattern-like ToNode materialization of AST nodes into
actual RegExpNode objects, where the compiler is passed in as an argument,
presumably to avoid paying for an extra zone_ field in every AST node. In most
cases, an available alternative would be to use on_success->zone(), which again
in practice refers to the very same zone.

I'd argue using on_success->zone() is actually more robust, since that makes the
lifetime of the return value of ToNode more obviously consistent, so I've now:

1. changed to on_success->zone()
2. moved the define (now called OZ) closer its first usage
3. added a comment
4. cc'ed zerny@ to verify this makes sense

[Ivan Posva, 2015-03-17 16:50:37]

LGTM

If Ian has any concerns we can address them at a later point.

-Ivan

[koda, 2015-03-17 19:25:09]

Committed patchset #5 (id:80001) manually as r44541 (presubmit successful).