Add Zone-based handle allocation interface and reduce use of Isolate-based interfaces.

Add Zone-based handle allocation interface and reduce use of Isolate-based interfaces.

Remove deprecated Isolate-based BitVector constructor.

R=asiva@google.com

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

[koda, 2015-01-22 23:50:22]

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

[Ivan Posva, 2015-01-23 04:53:03]

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

[Ivan Posva, 2015-01-23 04:53:04]

First round of comments.

-Ivan

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:143: Class::ZoneHandle(Z,
thread()->isolate()->object_store()->future_class());
Was wondering whether it makes sense to keep I defined as "thread()->isolate()",
but it seems to be the only use of isolate().

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
runtime/vm/flow_graph_allocator.cc:2058: AddPhi(input_phi);
We are calling Thread::Current() inside a doubly nested loop here, probably
better to either add the current thread to the ReachingDefs class or pass it
along down to AddPhi from an outer level

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
runtime/vm/flow_graph_allocator.cc:2164: if
(!reaching_defs_.Get(phi)->Contains(unallocated->vreg())) {
ditto for Get(phi)

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:98: return CheckedHandle(isolate->current_zone(), raw_ptr); 
                  \
I thought we had switched to isolate->zone() from current_zone() in one of our
previous reviews.

[koda, 2015-01-23 14:29:31]

PTAL

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/ast_transform...
File runtime/vm/ast_transformer.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/ast_transform...
runtime/vm/ast_transformer.cc:143: Class::ZoneHandle(Z,
thread()->isolate()->object_store()->future_class());
On 2015/01/23 04:53:04, Ivan Posva wrote:
> Was wondering whether it makes sense to keep I defined as
"thread()->isolate()",
> but it seems to be the only use of isolate().

Acknowledged.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
runtime/vm/flow_graph_allocator.cc:2058: AddPhi(input_phi);
On 2015/01/23 04:53:04, Ivan Posva wrote:
> We are calling Thread::Current() inside a doubly nested loop here, probably
> better to either add the current thread to the ReachingDefs class or pass it
> along down to AddPhi from an outer level

Done. Simply used the flow graph's zone.

Also got rid of a couple of Isolate::Current()->current_zone() the same way.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
runtime/vm/flow_graph_allocator.cc:2164: if
(!reaching_defs_.Get(phi)->Contains(unallocated->vreg())) {
Get doesn't need any zone.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:98: return CheckedHandle(isolate->current_zone(), raw_ptr); 
                  \
On 2015/01/23 04:53:04, Ivan Posva wrote:
> I thought we had switched to isolate->zone() from current_zone() in one of our
> previous reviews.

No, that renaming concerned Thread::zone. Since Isolate::current_zone is being
eliminated, I don't see much point in renaming it now (unless it's for adding a
"_deprecated" warning to its name).

[siva, 2015-01-23 19:29:27]

LGTM with some comments.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
runtime/vm/flow_graph_allocator.cc:2028: Zone* zone = Thread::Current()->zone();
Zone* zone = flow_graph_.zone();
here?

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/handles_impl.h
File runtime/vm/handles_impl.h (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/handles_impl....
runtime/vm/handles_impl.h:96: Isolate* isolate = Isolate::Current();
I think this should be
Thread* thread = Thread::Current();
ASSERT(thread != NULL);
ASSERT(thread->current_zone() == zone);

I think we should get rid of current_zone() in Isolate.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/handles_impl....
runtime/vm/handles_impl.h:115: Isolate* isolate = Isolate::Current();
Ditto here Thread::Current();

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:376: static Object& ZoneHandle(Isolate* isolate, RawObject*
raw_ptr) {
DEPRECATED tag for this too.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:835: static PassiveObject& Handle(Isolate* I, RawObject*
raw_ptr) {
Deprecated tag for this too.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:848: static PassiveObject& Handle(Isolate* I) {
Deprecated tag.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:851: static PassiveObject& ZoneHandle(Isolate* I, RawObject*
raw_ptr) {
Deprecated tag?

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

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:66: #define I (thread()->isolate())
Is I still used?

[Ivan Posva, 2015-01-23 19:49:25]

-Ivan

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/constant_prop...
File runtime/vm/constant_propagator.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/constant_prop...
runtime/vm/constant_propagator.cc:33: reachable_(new(graph->zone()) BitVector(
How about defining Z in this file?

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:685: new(zone())
InlineExitCollector(caller_graph_, call);
#define Z (zone())

[koda, 2015-01-23 21:09:43]

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
File runtime/vm/flow_graph_allocator.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_al...
runtime/vm/flow_graph_allocator.cc:2028: Zone* zone = Thread::Current()->zone();
On 2015/01/23 19:29:26, siva wrote:
> Zone* zone = flow_graph_.zone();
> here?

Done.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_in...
File runtime/vm/flow_graph_inliner.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/flow_graph_in...
runtime/vm/flow_graph_inliner.cc:685: new(zone())
InlineExitCollector(caller_graph_, call);
On 2015/01/23 19:49:24, Ivan Posva wrote:
> #define Z (zone())

Done.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/handles_impl.h
File runtime/vm/handles_impl.h (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/handles_impl....
runtime/vm/handles_impl.h:96: Isolate* isolate = Isolate::Current();
On 2015/01/23 19:29:27, siva wrote:
> I think this should be
> Thread* thread = Thread::Current();
> ASSERT(thread != NULL);
> ASSERT(thread->current_zone() == zone);
> 
> I think we should get rid of current_zone() in Isolate.

Agreed. That's why I already marked it as obsolete :)

Done.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/handles_impl....
runtime/vm/handles_impl.h:115: Isolate* isolate = Isolate::Current();
On 2015/01/23 19:29:27, siva wrote:
> Ditto here Thread::Current();

Done.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:376: static Object& ZoneHandle(Isolate* isolate, RawObject*
raw_ptr) {
On 2015/01/23 19:29:27, siva wrote:
> DEPRECATED tag for this too.

Done.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:835: static PassiveObject& Handle(Isolate* I, RawObject*
raw_ptr) {
On 2015/01/23 19:29:27, siva wrote:
> Deprecated tag for this too.

Done.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:848: static PassiveObject& Handle(Isolate* I) {
On 2015/01/23 19:29:27, siva wrote:
> Deprecated tag.

Done.

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/object.h#newc...
runtime/vm/object.h:851: static PassiveObject& ZoneHandle(Isolate* I, RawObject*
raw_ptr) {
On 2015/01/23 19:29:27, siva wrote:
> Deprecated tag?

Done.

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

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/parser.cc#new...
runtime/vm/parser.cc:66: #define I (thread()->isolate())
On 2015/01/23 19:29:27, siva wrote:
> Is I still used?

Yes in ~15 places, mostly to access the object_store.

[koda, 2015-01-23 21:12:10]

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/constant_prop...
File runtime/vm/constant_propagator.cc (right):

https://codereview.chromium.org/868913002/diff/60001/runtime/vm/constant_prop...
runtime/vm/constant_propagator.cc:33: reachable_(new(graph->zone()) BitVector(
On 2015/01/23 19:49:24, Ivan Posva wrote:
> How about defining Z in this file?

Done.

[koda, 2015-01-26 14:54:22]

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