Introduce simple Thread class to support gradual refactoring of interfaces.

Introduce simple Thread class to support gradual refactoring of interfaces.

This first iteration of Thread just forwards a subset of the BaseIsolate methods.

The plan is to first add Thread/Zone-based interfaces where appropriate, deprecate their Isolate-based versions, and finally remove them once all callsites have been migrated.

This CL only demonstrates a small part of this migration, for BitVector and some of the compiler classes. There are thousands of additional call-sites that will need to be updated.

R=asiva@google.com

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

[koda, 2015-01-21 21:39:43]

... and next on the list is to move handle allocation interfaces from taking an
Isolate to taking a Thread.

[Ivan Posva, 2015-01-21 21:53:13]

On 2015/01/21 21:39:43, koda wrote:
> ... and next on the list is to move handle allocation interfaces from taking
an
> Isolate to taking a Thread.

Why would the handle allocation interface not take a zone?

-Ivan

[koda, 2015-01-21 21:58:52]

That was my initial thinking as well, but there is an issue with the "no handle
allocation scope".

We'd like to be able to have a NoHandleScope even if there's no current zone (I
assume), which means the depth counter needs to live on the thread. Of course,
this is just for debugging, so maybe we are OK with paying the cost of doing the
thread-local lookup in that part of the code that checks that depth counter. If
so, then yes, we can use a pure Zone rather than Thread.

[siva, 2015-01-21 22:14:51]

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/allocation.h
File runtime/vm/allocation.h (right):

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/allocation.h#...
runtime/vm/allocation.h:99: void* operator new(uword size, BaseIsolate*
isolate);
if deprecated would it make sense to move it under 'private:' and friend the few
classes that still use this version.

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

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:431: live_out_.Add(new(zone()) BitVector(zone(),
variable_count_));
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.

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/thread.h#newc...
runtime/vm/thread.h:32: }
Missing the boiler plate DISALLOW stuff.

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

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/allocation.h
File runtime/vm/allocation.h (right):

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/allocation.h#...
runtime/vm/allocation.h:99: void* operator new(uword size, BaseIsolate*
isolate);
On 2015/01/21 22:14:50, siva wrote:
> if deprecated would it make sense to move it under 'private:' and friend the
few
> classes that still use this version.

There are still 10+ classes using it, at least one of which is an inner class,
which AFAIK cannot be forward-declared as a friend.

When the list is shorter and have no inner classes, I will do it.

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

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:431: live_out_.Add(new(zone()) BitVector(zone(),
variable_count_));
On 2015/01/21 22:14:50, siva wrote:
> 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.

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.

[koda, 2015-01-21 23:06:25]

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/thread.h
File runtime/vm/thread.h (right):

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/thread.h#newc...
runtime/vm/thread.h:32: }
On 2015/01/21 22:14:50, siva wrote:
> Missing the boiler plate DISALLOW stuff.

Done.

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

LGTM.

Ivan has a comment about Zone::Current which I think is valid, maybe you want to
wait for his review also.

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

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:431: live_out_.Add(new(zone()) BitVector(zone(),
variable_count_));
On 2015/01/21 23:05:44, koda wrote:
> On 2015/01/21 22:14:50, siva wrote:
> > 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.
> 
> 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.

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:43]

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

[Ivan Posva, 2015-01-22 01:26:45]

-Ivan

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

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:431: live_out_.Add(new(zone()) BitVector(zone(),
variable_count_));
On 2015/01/22 01:18:20, siva wrote:
> On 2015/01/21 23:05:44, koda wrote:
> > On 2015/01/21 22:14:50, siva wrote:
> > > 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.
> > 
> > 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.
> 
> 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).

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.

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/base_isolate....
runtime/vm/base_isolate.h:22: // DEPRECATED: Use Thread::current_zone.
Thread::Current()->zone()?

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:374: : zone_(Zone::Current()),
Thread::Current()->zone()

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/flow_graph.h#...
runtime/vm/flow_graph.h:154: Zone* zone() const { return
thread()->current_zone(); }
I find current_zone() a bit strange.

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/thread.h#newc...
runtime/vm/thread.h:25: Zone* current_zone() {
current_zone() -> zone()

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/zone.h#newcode61
runtime/vm/zone.h:61: static Zone* Current();
I don't think this adds much.

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

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

https://codereview.chromium.org/850183005/diff/40001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:431: live_out_.Add(new(zone()) BitVector(zone(),
variable_count_));
On 2015/01/22 01:26:44, Ivan Posva wrote:
> On 2015/01/22 01:18:20, siva wrote:
> > On 2015/01/21 23:05:44, koda wrote:
> > > On 2015/01/21 22:14:50, siva wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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).
> 
> 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.

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.

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/base_isolate....
runtime/vm/base_isolate.h:22: // DEPRECATED: Use Thread::current_zone.
On 2015/01/22 01:26:44, Ivan Posva wrote:
> Thread::Current()->zone()?

Done.

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/flow_graph.cc...
runtime/vm/flow_graph.cc:374: : zone_(Zone::Current()),
On 2015/01/22 01:26:44, Ivan Posva wrote:
> Thread::Current()->zone()

Done.

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/flow_graph.h#...
runtime/vm/flow_graph.h:154: Zone* zone() const { return
thread()->current_zone(); }
On 2015/01/22 01:26:44, Ivan Posva wrote:
> I find current_zone() a bit strange.

Done.

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/thread.h#newc...
runtime/vm/thread.h:25: Zone* current_zone() {
On 2015/01/22 01:26:44, Ivan Posva wrote:
> current_zone() -> zone()

Done.

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

https://codereview.chromium.org/850183005/diff/60001/runtime/vm/zone.h#newcode61
runtime/vm/zone.h:61: static Zone* Current();
On 2015/01/22 01:26:44, Ivan Posva wrote:
> I don't think this adds much.

Done.

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

Committed patchset #6 (id:100001) manually as r43073 (presubmit successful).