Remove TLS access for current Zone.

Remove TLS access for current Zone.

By passing around a Zone object explicitly we no longer need to do a
TLS access at the sites that allocate memory from the current Zone.

BUG=
TEST=

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

[sanjoy, 2012-06-05 13:02:49]

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc#newcode6926
src/hydrogen.cc:6926: AddInstruction(global);
This needs to be done so that the HGlobalReceiver constructor sees a properly
initialized HValue instruction, with a non-NULL parent HBasicBlock.

[danno, 2012-06-05 13:42:33]

Mostly nits, but a few real issues to address.

http://codereview.chromium.org/10534006/diff/1/src/ast.cc
File src/ast.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/ast.cc#newcode248
src/ast.cc:248: ZoneHashMap table(Literal::Match, 8, allocator);
Please use a constant for this. Didn't you add one in your last CL?

http://codereview.chromium.org/10534006/diff/1/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/compiler.cc#newcode351
src/compiler.cc:351: info->isolate()->zone());
How about add a zone() utility to CompileInfo? This is used a lot in this file.

http://codereview.chromium.org/10534006/diff/1/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/full-codegen.cc#newcode306
src/full-codegen.cc:306: FullCodeGenerator cgen(&masm, info, isolate->zone());
use zone() directly, since the FullCodeGenerator has a direct reference.

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc#newcode3138
src/hydrogen.cc:3138: explicit BoundsCheckTable(Zone* zone) :
ZoneHashMap(BoundsCheckKeyMatch, 8,
Make this a shared constant. Also, weird formatting. Put the : on the next line
and indent accordingly

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc#newcode5597
src/hydrogen.cc:5597: AddInstruction(HCheckInstanceType::NewIsSpecObject(object,
graph()->zone()));
use just zone()

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc#newcode6313
src/hydrogen.cc:6313: new(isolate()->zone())
HConstant(Handle<Context>(target->context()),
why not just zone()?

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.h#newcode617
src/hydrogen.h:617: inline Zone* zone() const;
Why the const here and not on other version of zone() on other objects?

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.h#newcode1154
src/hydrogen.h:1154: Zone* zone() const { return zone_; }
Why the const here and not on other version of zone() on other objects?

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.h#newcode1186
src/hydrogen.h:1186: Zone* AstContext::zone() const { return owner_->zone(); }
Why the const here and not on other version of zone() on other objects?

http://codereview.chromium.org/10534006/diff/1/src/interface.cc
File src/interface.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/interface.cc#newcode93
src/interface.cc:93: *map = new ZoneHashMap(Match, 8, allocator);
Constant for default argument

http://codereview.chromium.org/10534006/diff/1/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/jsregexp.cc#newcode4374
src/jsregexp.cc:4374: ZoneAllocationPolicy allocator(that->zone());
Why do you need allocator?

http://codereview.chromium.org/10534006/diff/1/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/lithium-allocator.cc#newco...
src/lithium-allocator.cc:537: live_ranges_(num_values * 2, graph->zone()),
are you sure of initialization order? either use zone_ everywhere or
graph->zone() everywhere.

http://codereview.chromium.org/10534006/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/runtime.cc#newcode3741
src/runtime.cc:3741: ZoneList<int> offsets(8, isolate->zone());
just zone

http://codereview.chromium.org/10534006/diff/1/src/runtime.cc#newcode6463
src/runtime.cc:6463: ZoneList<int> indices(initial_capacity, isolate->zone());
just zone

http://codereview.chromium.org/10534006/diff/1/src/safepoint-table.cc
File src/safepoint-table.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/safepoint-table.cc#newcode142
src/safepoint-table.cc:142: ? new(assembler->zone()) ZoneList<int>(4,
assembler->zone())
just zone 2x

http://codereview.chromium.org/10534006/diff/1/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/10534006/diff/1/src/scopes.h#newcode43
src/scopes.h:43: explicit VariableMap(Zone* zone);
Seems weird that you have to pass this into the constructor and again in
Declare. Cache as member?

http://codereview.chromium.org/10534006/diff/1/src/splay-tree-inl.h
File src/splay-tree-inl.h (right):

http://codereview.chromium.org/10534006/diff/1/src/splay-tree-inl.h#newcode39
src/splay-tree-inl.h:39: // Pre-allocate some space for tiny trees.  Since we
don't have an
I don't understand this code. ForEachNode should take the allocator.

[sanjoy, 2012-06-05 14:21:39]

http://codereview.chromium.org/10534006/diff/1/src/ast.cc
File src/ast.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/ast.cc#newcode248
src/ast.cc:248: ZoneHashMap table(Literal::Match, 8, allocator);
On 2012/06/05 13:42:35, danno wrote:
> Please use a constant for this. Didn't you add one in your last CL?

Fixed.

http://codereview.chromium.org/10534006/diff/1/src/compiler.cc
File src/compiler.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/compiler.cc#newcode351
src/compiler.cc:351: info->isolate()->zone());
On 2012/06/05 13:42:35, danno wrote:
> How about add a zone() utility to CompileInfo? This is used a lot in this
file.

I'm adding that in the next CL.

http://codereview.chromium.org/10534006/diff/1/src/full-codegen.cc
File src/full-codegen.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/full-codegen.cc#newcode306
src/full-codegen.cc:306: FullCodeGenerator cgen(&masm, info, isolate->zone());
On 2012/06/05 13:42:35, danno wrote:
> use zone() directly, since the FullCodeGenerator has a direct reference.

This method is static.  I'll change this to use the Zone in the CompilationInfo
in the next CL.

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc#newcode3138
src/hydrogen.cc:3138: explicit BoundsCheckTable(Zone* zone) :
ZoneHashMap(BoundsCheckKeyMatch, 8,
On 2012/06/05 13:42:35, danno wrote:
> Make this a shared constant. Also, weird formatting. Put the : on the next
line
> and indent accordingly

Fixed.

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc#newcode5597
src/hydrogen.cc:5597: AddInstruction(HCheckInstanceType::NewIsSpecObject(object,
graph()->zone()));
On 2012/06/05 13:42:35, danno wrote:
> use just zone()

Fixed.

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.cc#newcode6313
src/hydrogen.cc:6313: new(isolate()->zone())
HConstant(Handle<Context>(target->context()),
On 2012/06/05 13:42:35, danno wrote:
> why not just zone()?

Fixed.

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.h
File src/hydrogen.h (right):

http://codereview.chromium.org/10534006/diff/1/src/hydrogen.h#newcode617
src/hydrogen.h:617: inline Zone* zone() const;
On 2012/06/05 13:42:35, danno wrote:
> Why the const here and not on other version of zone() on other objects?

Fixed.  Changed all the zone() methods (except the one in Isolate) to const.

http://codereview.chromium.org/10534006/diff/1/src/interface.cc
File src/interface.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/interface.cc#newcode93
src/interface.cc:93: *map = new ZoneHashMap(Match, 8, allocator);
On 2012/06/05 13:42:35, danno wrote:
> Constant for default argument

Fixed.

http://codereview.chromium.org/10534006/diff/1/src/jsregexp.cc
File src/jsregexp.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/jsregexp.cc#newcode4374
src/jsregexp.cc:4374: ZoneAllocationPolicy allocator(that->zone());
On 2012/06/05 13:42:35, danno wrote:
> Why do you need allocator?

Not needed. Fixed.

http://codereview.chromium.org/10534006/diff/1/src/lithium-allocator.cc
File src/lithium-allocator.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/lithium-allocator.cc#newco...
src/lithium-allocator.cc:537: live_ranges_(num_values * 2, graph->zone()),
On 2012/06/05 13:42:35, danno wrote:
> are you sure of initialization order? either use zone_ everywhere or
> graph->zone() everywhere.
Fixed.

http://codereview.chromium.org/10534006/diff/1/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/runtime.cc#newcode3741
src/runtime.cc:3741: ZoneList<int> offsets(8, isolate->zone());
On 2012/06/05 13:42:35, danno wrote:
> just zone

Fixed.

http://codereview.chromium.org/10534006/diff/1/src/runtime.cc#newcode6463
src/runtime.cc:6463: ZoneList<int> indices(initial_capacity, isolate->zone());
On 2012/06/05 13:42:35, danno wrote:
> just zone

Fixed.

http://codereview.chromium.org/10534006/diff/1/src/safepoint-table.cc
File src/safepoint-table.cc (right):

http://codereview.chromium.org/10534006/diff/1/src/safepoint-table.cc#newcode142
src/safepoint-table.cc:142: ? new(assembler->zone()) ZoneList<int>(4,
assembler->zone())
On 2012/06/05 13:42:35, danno wrote:
> just zone 2x

Fixed.

http://codereview.chromium.org/10534006/diff/1/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/10534006/diff/1/src/scopes.h#newcode43
src/scopes.h:43: explicit VariableMap(Zone* zone);
On 2012/06/05 13:42:35, danno wrote:
> Seems weird that you have to pass this into the constructor and again in
> Declare. Cache as member?

I figured not adding a member will save some space. :)

Moreover, Declare is called only once in a couple of places, and in all those
places I already have a Zone.

http://codereview.chromium.org/10534006/diff/1/src/splay-tree-inl.h
File src/splay-tree-inl.h (right):

http://codereview.chromium.org/10534006/diff/1/src/splay-tree-inl.h#newcode39
src/splay-tree-inl.h:39: // Pre-allocate some space for tiny trees.  Since we
don't have an
On 2012/06/05 13:42:35, danno wrote:
> I don't understand this code. ForEachNode should take the allocator.

Yes, but we don't have the allocator object in the destructor.

[danno, 2012-06-06 10:59:53]

More comments.

https://chromiumcodereview.appspot.com/10534006/diff/1/src/scopes.h
File src/scopes.h (right):

https://chromiumcodereview.appspot.com/10534006/diff/1/src/scopes.h#newcode43
src/scopes.h:43: explicit VariableMap(Zone* zone);
This is probably cleaner still if the zone is stored in the object.

On 2012/06/05 14:21:39, sanjoy wrote:
> On 2012/06/05 13:42:35, danno wrote:
> > Seems weird that you have to pass this into the constructor and again in
> > Declare. Cache as member?
> 
> I figured not adding a member will save some space. :)
> 
> Moreover, Declare is called only once in a couple of places, and in all those
> places I already have a Zone.

https://chromiumcodereview.appspot.com/10534006/diff/10001/src/splay-tree-inl.h
File src/splay-tree-inl.h (right):

https://chromiumcodereview.appspot.com/10534006/diff/10001/src/splay-tree-inl...
src/splay-tree-inl.h:49: }
Store the allocator inside the splay tree so that you can use the existing code.

[sanjoy, 2012-06-06 11:43:54]

http://codereview.chromium.org/10534006/diff/10001/src/splay-tree-inl.h
File src/splay-tree-inl.h (right):

http://codereview.chromium.org/10534006/diff/10001/src/splay-tree-inl.h#newco...
src/splay-tree-inl.h:49: }
On 2012/06/06 10:59:53, danno wrote:
> Store the allocator inside the splay tree so that you can use the existing
code.

Done.

[danno, 2012-06-11 12:11:45]

http://codereview.chromium.org/10534006/diff/3003/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10534006/diff/3003/src/hydrogen.cc#newcode6958
src/hydrogen.cc:6958: AddInstruction(global_object);
This is an unrelated change, can you please remove it from this patch?

http://codereview.chromium.org/10534006/diff/3003/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/10534006/diff/3003/src/parser.cc#newcode1557
src/parser.cc:1557: interface->Add(names[i], inner, ok, zone());
Move zone() to be the third parameter so that the CHECK_OK parameter still
works.

http://codereview.chromium.org/10534006/diff/3003/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/10534006/diff/3003/src/scopes.h#newcode72
src/scopes.h:72: maps_[i] = new VariableMap(zone);
Where does this allocation come from? It looks like it's from the malloc heap,
that doesn't seem right.

[danno, 2012-06-11 12:37:50]

lgtm

[sanjoy, 2012-06-11 12:37:56]

http://codereview.chromium.org/10534006/diff/3003/src/hydrogen.cc
File src/hydrogen.cc (right):

http://codereview.chromium.org/10534006/diff/3003/src/hydrogen.cc#newcode6958
src/hydrogen.cc:6958: AddInstruction(global_object);
On 2012/06/11 12:11:45, danno wrote:
> This is an unrelated change, can you please remove it from this patch?

This change is required since the HGlobalReceiver constructor reads the zone
from the HBasicBlock in the HValue.  Unless we add the instruction to the basic
block before constructing the HGlobalReceiver, the HGlobalReceiver constructor
sees a NULL HBasicBlock and segfaults.

http://codereview.chromium.org/10534006/diff/3003/src/parser.cc
File src/parser.cc (right):

http://codereview.chromium.org/10534006/diff/3003/src/parser.cc#newcode1557
src/parser.cc:1557: interface->Add(names[i], inner, ok, zone());
On 2012/06/11 12:11:45, danno wrote:
> Move zone() to be the third parameter so that the CHECK_OK parameter still
> works.

Fixed.

http://codereview.chromium.org/10534006/diff/3003/src/scopes.h
File src/scopes.h (right):

http://codereview.chromium.org/10534006/diff/3003/src/scopes.h#newcode72
src/scopes.h:72: maps_[i] = new VariableMap(zone);
On 2012/06/11 12:11:45, danno wrote:
> Where does this allocation come from? It looks like it's from the malloc heap,
> that doesn't seem right.

Changed to allocate from the zone.  VariableMap isn't a ZoneObject, so I had to
explicitly allocate the memory from the zone.