Progress towards making Zones independent of Isolates and Threads.

Progress towards making Zones independent of Isolates and Threads.

This CL changes some parts of the code to explicitly pass around a
Zone.  Not passing in a zone is okay too (in fact most of v8 still
doesn't), but that may incur a TLS lookup.

BUG=
TEST=

Committed: https://code.google.com/p/v8/source/detail?r=11709

[danno, 2012-06-01 11:29:24]

http://codereview.chromium.org/10443114/diff/1/src/hashmap.h
File src/hashmap.h (right):

http://codereview.chromium.org/10443114/diff/1/src/hashmap.h#newcode310
src/hashmap.h:310: 
nit: remove stray extra line

http://codereview.chromium.org/10443114/diff/3002/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/10443114/diff/3002/src/ia32/lithium-ia32.h#new...
src/ia32/lithium-ia32.h:2377: zone_(zone),
You should explicitly store the zone in the graph and get it from graph->zone()
rather than passing it in here.

http://codereview.chromium.org/10443114/diff/3002/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/10443114/diff/3002/src/parser.cc#newcode5276
src/parser.cc:5276: captures_started(), zone());
nit: strange indentation

http://codereview.chromium.org/10443114/diff/3002/src/parser.cc#newcode6028
src/parser.cc:6028: Parser parser(script, parsing_flags, NULL, NULL,
info->isolate()->zone());
Seem's you don't want to use the isolate here. How about putting the zone in the
CompilationInfo?

http://codereview.chromium.org/10443114/diff/3002/src/parser.cc#newcode6037
src/parser.cc:6037: info->isolate()->zone());
Same here.

http://codereview.chromium.org/10443114/diff/3002/src/splay-tree.h
File src/splay-tree.h (left):

http://codereview.chromium.org/10443114/diff/3002/src/splay-tree.h#oldcode197
src/splay-tree.h:197: 
nit: add empty line back

http://codereview.chromium.org/10443114/diff/3002/src/x64/lithium-codegen-x64.h
File src/x64/lithium-codegen-x64.h (right):

http://codereview.chromium.org/10443114/diff/3002/src/x64/lithium-codegen-x64...
src/x64/lithium-codegen-x64.h:48: LCodeGen(LChunk* chunk, MacroAssembler*
assembler, CompilationInfo* info,
Perhaps it's better to put the zone in the macro-assembler? That's used used in
several places.

http://codereview.chromium.org/10443114/diff/3002/src/x64/lithium-x64.h
File src/x64/lithium-x64.h (right):

http://codereview.chromium.org/10443114/diff/3002/src/x64/lithium-x64.h#newco...
src/x64/lithium-x64.h:2235: zone_(zone),
Put the zone in the graph, get it from there.

http://codereview.chromium.org/10443114/diff/3002/src/zone-inl.h
File src/zone-inl.h (right):

http://codereview.chromium.org/10443114/diff/3002/src/zone-inl.h#newcode109
src/zone-inl.h:109: if (zone_)
nit: always use {}, even if the block is only one line

http://codereview.chromium.org/10443114/diff/3002/src/zone-inl.h#newcode112
src/zone-inl.h:112: return ZONE->New(size);
same here.

http://codereview.chromium.org/10443114/diff/3002/src/zone.h
File src/zone.h (right):

http://codereview.chromium.org/10443114/diff/3002/src/zone.h#newcode155
src/zone.h:155: Zone* zone_;
Put this back where it was later in the file. to make diffs easier.

http://codereview.chromium.org/10443114/diff/3002/src/zone.h#newcode202
src/zone.h:202: // We override almost all List's public functions here so that
the
It's not really overriding. You're adding convenience function wrapper.

http://codereview.chromium.org/10443114/diff/3002/test/cctest/test-list.cc
File test/cctest/test-list.cc (right):

http://codereview.chromium.org/10443114/diff/3002/test/cctest/test-list.cc#ne...
test/cctest/test-list.cc:36: struct ZeroingAllocationPolicy {
This is an unrelated, unnecessary change. Please remove it.

[sanjoy, 2012-06-01 11:39:26]

http://codereview.chromium.org/10443114/diff/3002/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/10443114/diff/3002/src/parser.cc#newcode6028
src/parser.cc:6028: Parser parser(script, parsing_flags, NULL, NULL,
info->isolate()->zone());
On 2012/06/01 11:29:24, danno wrote:
> Seem's you don't want to use the isolate here. How about putting the zone in
the
> CompilationInfo?

I'm delegating this sort of change to a later patch where I'll remove the zone()
method of Isolate altogether.  That should automatically catch all the places
were I implicitly use the Zone in the Isolate.

[danno, 2012-06-01 15:25:34]

It doesn't look like you've ported to ARM or MIPS in this CL yet?

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/hydrogen.cc#ne...
src/hydrogen.cc:699: LChunkBuilder builder(info, this, &allocator,
info->isolate()->zone());
Please put the zone in the graph. It's a simple change that I think makes sense
even now.

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/ia32/lithium-i...
File src/ia32/lithium-ia32.h (right):

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/ia32/lithium-i...
src/ia32/lithium-ia32.h:2377: zone_(zone),
get it directly from the zone

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/jsregexp.cc
File src/jsregexp.cc (right):

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/jsregexp.cc#ne...
src/jsregexp.cc:647: zone);
nit: weird indentation

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/list.h
File src/list.h (right):

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/list.h#newcode48
src/list.h:48: // template <typename T, class P = FreeStoreAllocationPolicy>
class List;
May want to change the comment to use AllocationPolicy

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/splay-tree.h
File src/splay-tree.h (right):

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/splay-tree.h#n...
src/splay-tree.h:190: explicit NodeDeleter() {
Why is this change needed? I don't think you should add explicit here.

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/v8globals.h
File src/v8globals.h (right):

https://chromiumcodereview.appspot.com/10443114/diff/11001/src/v8globals.h#ne...
src/v8globals.h:159: template <typename Config, class AllocationPolicy =
FreeStoreAllocationPolicy>
No need to change this file for just this one-line change.

[sanjoy, 2012-06-04 10:17:21]

Ported to changes to arm.

http://codereview.chromium.org/10443114/diff/11001/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10443114/diff/11001/src/hydrogen.cc#newcode699
src/hydrogen.cc:699: LChunkBuilder builder(info, this, &allocator,
info->isolate()->zone());
On 2012/06/01 15:25:34, danno wrote:
> Please put the zone in the graph. It's a simple change that I think makes
sense
> even now.

Done.

http://codereview.chromium.org/10443114/diff/11001/src/ia32/lithium-ia32.h
File src/ia32/lithium-ia32.h (right):

http://codereview.chromium.org/10443114/diff/11001/src/ia32/lithium-ia32.h#ne...
src/ia32/lithium-ia32.h:2377: zone_(zone),
On 2012/06/01 15:25:34, danno wrote:
> get it directly from the zone

Changed it to directly read the Zone off the HGraph.

http://codereview.chromium.org/10443114/diff/11001/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/10443114/diff/11001/src/jsregexp.cc#newcode647
src/jsregexp.cc:647: zone);
On 2012/06/01 15:25:34, danno wrote:
> nit: weird indentation

Fixed.

http://codereview.chromium.org/10443114/diff/11001/src/list.h
File src/list.h (right):

http://codereview.chromium.org/10443114/diff/11001/src/list.h#newcode48
src/list.h:48: // template <typename T, class P = FreeStoreAllocationPolicy>
class List;
On 2012/06/01 15:25:34, danno wrote:
> May want to change the comment to use AllocationPolicy

Done.

http://codereview.chromium.org/10443114/diff/11001/src/splay-tree.h
File src/splay-tree.h (right):

http://codereview.chromium.org/10443114/diff/11001/src/splay-tree.h#newcode190
src/splay-tree.h:190: explicit NodeDeleter() {
On 2012/06/01 15:25:34, danno wrote:
> Why is this change needed? I don't think you should add explicit here.

Removed it.

http://codereview.chromium.org/10443114/diff/11001/src/v8globals.h
File src/v8globals.h (right):

http://codereview.chromium.org/10443114/diff/11001/src/v8globals.h#newcode159
src/v8globals.h:159: template <typename Config, class AllocationPolicy =
FreeStoreAllocationPolicy>
On 2012/06/01 15:25:34, danno wrote:
> No need to change this file for just this one-line change.

Reverted the change.

[danno, 2012-06-04 10:46:46]

http://codereview.chromium.org/10443114/diff/10002/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10443114/diff/10002/src/compiler.cc#newcode307
src/compiler.cc:307: Handle<Code> optimized_code = graph->Compile(info,
info->isolate()->zone());
either graph->zone() or put the zone into CompilationInfo.

http://codereview.chromium.org/10443114/diff/10002/src/deoptimizer.h
File src/deoptimizer.h (right):

http://codereview.chromium.org/10443114/diff/10002/src/deoptimizer.h#newcode609
src/deoptimizer.h:609: 
nit: no space here, stay consistent with surrounding formatting.

http://codereview.chromium.org/10443114/diff/10002/src/hashmap.h
File src/hashmap.h (right):

http://codereview.chromium.org/10443114/diff/10002/src/hashmap.h#newcode45
src/hashmap.h:45: TemplateHashMapImpl(MatchFun match, uint32_t capacity = 8,
Turn default value into a constant so that you can specify the default constant
when explicitly calling with an allocation.

http://codereview.chromium.org/10443114/diff/10002/src/hashmap.h#newcode339
src/hashmap.h:339: : TemplateHashMapImpl<AllocationPolicy>(match, 8, allocator)
{ }
As discussed above, use constant for this.

http://codereview.chromium.org/10443114/diff/10002/src/parser.h
File src/parser.h (right):

http://codereview.chromium.org/10443114/diff/10002/src/parser.h#newcode838
src/parser.h:838: 
No extra space here to match surrounding code.

[sanjoy, 2012-06-04 11:05:42]

http://codereview.chromium.org/10443114/diff/10002/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10443114/diff/10002/src/compiler.cc#newcode307
src/compiler.cc:307: Handle<Code> optimized_code = graph->Compile(info,
info->isolate()->zone());
On 2012/06/04 10:46:46, danno wrote:
> either graph->zone() or put the zone into CompilationInfo.

Changed to graph->zone()

http://codereview.chromium.org/10443114/diff/10002/src/deoptimizer.h
File src/deoptimizer.h (right):

http://codereview.chromium.org/10443114/diff/10002/src/deoptimizer.h#newcode609
src/deoptimizer.h:609: 
On 2012/06/04 10:46:46, danno wrote:
> nit: no space here, stay consistent with surrounding formatting.

Removed space.

http://codereview.chromium.org/10443114/diff/10002/src/hashmap.h
File src/hashmap.h (right):

http://codereview.chromium.org/10443114/diff/10002/src/hashmap.h#newcode45
src/hashmap.h:45: TemplateHashMapImpl(MatchFun match, uint32_t capacity = 8,
On 2012/06/04 10:46:46, danno wrote:
> Turn default value into a constant so that you can specify the default
constant
> when explicitly calling with an allocation.

Done.

http://codereview.chromium.org/10443114/diff/10002/src/hashmap.h#newcode339
src/hashmap.h:339: : TemplateHashMapImpl<AllocationPolicy>(match, 8, allocator)
{ }
On 2012/06/04 10:46:46, danno wrote:
> As discussed above, use constant for this.

Done.

http://codereview.chromium.org/10443114/diff/10002/src/parser.h
File src/parser.h (right):

http://codereview.chromium.org/10443114/diff/10002/src/parser.h#newcode838
src/parser.h:838: 
On 2012/06/04 10:46:46, danno wrote:
> No extra space here to match surrounding code.

Done.

[danno, 2012-06-04 13:46:17]

LGTM with nits.

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/hashmap.h
File src/hashmap.h (right):

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/hashmap.h#newc...
src/hashmap.h:345: : TemplateHashMapImpl<AllocationPolicy>(
nit: two space indent from beginning of line here, and the following lines lined
up accordingly.

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/zone.h
File src/zone.h (right):

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/zone.h#newcode167
src/zone.h:167: // The ZoneAllocationPolicy is used to specialize the generic
data
nit: should be just "specialize generic data"

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/zone.h#newcode202
src/zone.h:202: // We add some convenience wrappers where so that we can pass in
a
nit: should be just "wrappers so that"

[sanjoy, 2012-06-04 13:58:53]

Fixed nits and rebased on bleeding_edge.

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/hashmap.h
File src/hashmap.h (right):

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/hashmap.h#newc...
src/hashmap.h:345: : TemplateHashMapImpl<AllocationPolicy>(
On 2012/06/04 13:46:17, danno wrote:
> nit: two space indent from beginning of line here, and the following lines
lined
> up accordingly.

Fixed.

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/zone.h
File src/zone.h (right):

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/zone.h#newcode167
src/zone.h:167: // The ZoneAllocationPolicy is used to specialize the generic
data
On 2012/06/04 13:46:17, danno wrote:
> nit: should be just "specialize generic data"

Done.

https://chromiumcodereview.appspot.com/10443114/diff/20003/src/zone.h#newcode202
src/zone.h:202: // We add some convenience wrappers where so that we can pass in
a
On 2012/06/04 13:46:17, danno wrote:
> nit: should be just "wrappers so that"

Done.