One Zone per CompilationInfo.

One Zone per CompilationInfo.

The CompilationInfo record now saves a Zone, and the compiler pipeline
allocates memory from the Zone in the CompilationInfo.  Before
compiling a function, we create a Zone on the stack and save a pointer
to that Zone to the CompilationInfo; which then gets picked up and
allocated from.

BUG=
TEST=

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

[sanjoy, 2012-06-13 15:29:10]

Deleted the previous CL -- it had a memory leak.

[danno, 2012-06-14 14:22:18]

http://codereview.chromium.org/10534139/diff/1/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):

http://codereview.chromium.org/10534139/diff/1/src/arm/lithium-codegen-arm.h#...
src/arm/lithium-codegen-arm.h:54: deoptimizations_(4, zone_),
This is really fragile, since it depends on order of initialization. Use
info->zone() instead here and below.

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

http://codereview.chromium.org/10534139/diff/1/src/compiler.cc#newcode533
src/compiler.cc:533: ZoneScope scope(&zone, DELETE_ON_EXIT);
Here and elsewhere, shouldn't the ZoneScope be directly after the Zone
declaration?

http://codereview.chromium.org/10534139/diff/1/src/compiler.cc#newcode582
src/compiler.cc:582: CompilationInfo info(script, &zone);
If the Zone and ZoneScope's lifecycle is tied to the lifecycle of the
CompilationInfo, why not make them members instead. That way you only have to
declare the CompilationInfo at each site and not all three.

http://codereview.chromium.org/10534139/diff/1/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10534139/diff/1/src/compiler.h#newcode259
src/compiler.h:259: // allocates from.
Maybe: The zone from which the compilation pipeline working on this
CompilationInfo allocates.

http://codereview.chromium.org/10534139/diff/1/src/ia32/lithium-codegen-ia32.h
File src/ia32/lithium-codegen-ia32.h (right):

http://codereview.chromium.org/10534139/diff/1/src/ia32/lithium-codegen-ia32....
src/ia32/lithium-codegen-ia32.h:57: deoptimizations_(4, zone_),
use info->zone(), initialization order dependency

http://codereview.chromium.org/10534139/diff/1/src/mips/lithium-codegen-mips.h
File src/mips/lithium-codegen-mips.h (right):

http://codereview.chromium.org/10534139/diff/1/src/mips/lithium-codegen-mips....
src/mips/lithium-codegen-mips.h:54: deoptimizations_(4, zone_),
info->zone() here and below

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

http://codereview.chromium.org/10534139/diff/1/src/runtime.cc#newcode1191
src/runtime.cc:1191: ZoneScope zone_scope(isolate->runtime_zone(),
DELETE_ON_EXIT);
Why new new ZoneScope here?

http://codereview.chromium.org/10534139/diff/1/src/runtime.cc#newcode11152
src/runtime.cc:11152: Zone zone(isolate);
Why put this here and not closer to the CompilationInfos? If you put the Zone
and ZoneScope directly into the CompilationInfo, you can even get rid of these.

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

http://codereview.chromium.org/10534139/diff/1/src/x64/lithium-codegen-x64.h#...
src/x64/lithium-codegen-x64.h:56: deoptimizations_(4, zone_),
info->zone()

http://codereview.chromium.org/10534139/diff/1/src/zone.cc
File src/zone.cc (right):

http://codereview.chromium.org/10534139/diff/1/src/zone.cc#newcode108
src/zone.cc:108: if (isolate_->runtime_zone() == this)
Yikes! Please move the DeleteAll(DELETE_ALL_BLOCKS) into ~Zone(), and call
DeleteAll(DELETE_ALL_BUT_LAST_BLOCK) where it used to be called.

http://codereview.chromium.org/10534139/diff/1/src/zone.cc#newcode134
src/zone.cc:134: void Zone::DeleteAllButOne() {
Just add a enum flag to DeleteAll that handles the last block specially.

http://codereview.chromium.org/10534139/diff/1/src/zone.h
File src/zone.h (right):

http://codereview.chromium.org/10534139/diff/1/src/zone.h#newcode78
src/zone.h:78: void Destroy();
Why not just make sure that the destructor of the Zone cleans up everything? In
the isolate case, the destructor never gets called and you will always keep
around the extra small segment.

[sanjoy, 2012-06-15 09:24:30]

http://codereview.chromium.org/10534139/diff/1/src/arm/lithium-codegen-arm.h
File src/arm/lithium-codegen-arm.h (right):

http://codereview.chromium.org/10534139/diff/1/src/arm/lithium-codegen-arm.h#...
src/arm/lithium-codegen-arm.h:54: deoptimizations_(4, zone_),
On 2012/06/14 14:22:19, danno wrote:
> This is really fragile, since it depends on order of initialization. Use
> info->zone() instead here and below.

Done.

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

http://codereview.chromium.org/10534139/diff/1/src/compiler.cc#newcode533
src/compiler.cc:533: ZoneScope scope(&zone, DELETE_ON_EXIT);
On 2012/06/14 14:22:19, danno wrote:
> Here and elsewhere, shouldn't the ZoneScope be directly after the Zone
> declaration?

CompilationInfo doesn't care.  I found this order more intuitive:

We create a zone, we create a CompilationInfo tied to the Zone and then we open
the Zone for allocation.

http://codereview.chromium.org/10534139/diff/1/src/compiler.cc#newcode582
src/compiler.cc:582: CompilationInfo info(script, &zone);
On 2012/06/14 14:22:19, danno wrote:
> If the Zone and ZoneScope's lifecycle is tied to the lifecycle of the
> CompilationInfo, why not make them members instead. That way you only have to
> declare the CompilationInfo at each site and not all three.

Please see my comment on runtime.cc

http://codereview.chromium.org/10534139/diff/1/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10534139/diff/1/src/compiler.h#newcode259
src/compiler.h:259: // allocates from.
On 2012/06/14 14:22:19, danno wrote:
> Maybe: The zone from which the compilation pipeline working on this
> CompilationInfo allocates.

Fixed.

http://codereview.chromium.org/10534139/diff/1/src/ia32/lithium-codegen-ia32.h
File src/ia32/lithium-codegen-ia32.h (right):

http://codereview.chromium.org/10534139/diff/1/src/ia32/lithium-codegen-ia32....
src/ia32/lithium-codegen-ia32.h:57: deoptimizations_(4, zone_),
On 2012/06/14 14:22:19, danno wrote:
> use info->zone(), initialization order dependency

Done.

http://codereview.chromium.org/10534139/diff/1/src/mips/lithium-codegen-mips.h
File src/mips/lithium-codegen-mips.h (right):

http://codereview.chromium.org/10534139/diff/1/src/mips/lithium-codegen-mips....
src/mips/lithium-codegen-mips.h:54: deoptimizations_(4, zone_),
On 2012/06/14 14:22:19, danno wrote:
> info->zone() here and below

Done.

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

http://codereview.chromium.org/10534139/diff/1/src/runtime.cc#newcode1191
src/runtime.cc:1191: ZoneScope zone_scope(isolate->runtime_zone(),
DELETE_ON_EXIT);
On 2012/06/14 14:22:19, danno wrote:
> Why new new ZoneScope here?

This runtime function is sometimes called without installing a ZoneScope, and it
is always safe (and cheap) to create a new ZoneScope.  But after Yang's change,
I've moved this to the Regexp code, where I think it makes more sense.

http://codereview.chromium.org/10534139/diff/1/src/runtime.cc#newcode11152
src/runtime.cc:11152: Zone zone(isolate);
On 2012/06/14 14:22:19, danno wrote:
> Why put this here and not closer to the CompilationInfos? If you put the Zone
> and ZoneScope directly into the CompilationInfo, you can even get rid of
these.

Because below a Scope allocated inside a Zone is being used outside the lexical
scope of the corresponding CompilationInfo (line 11210, 11219).

Plus, I think it keeps the design a little bit more flexible.

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

http://codereview.chromium.org/10534139/diff/1/src/x64/lithium-codegen-x64.h#...
src/x64/lithium-codegen-x64.h:56: deoptimizations_(4, zone_),
On 2012/06/14 14:22:19, danno wrote:
> info->zone()

Fixed.

http://codereview.chromium.org/10534139/diff/1/src/zone.cc
File src/zone.cc (right):

http://codereview.chromium.org/10534139/diff/1/src/zone.cc#newcode108
src/zone.cc:108: if (isolate_->runtime_zone() == this)
On 2012/06/14 14:22:19, danno wrote:
> Yikes! Please move the DeleteAll(DELETE_ALL_BLOCKS) into ~Zone(), and call
> DeleteAll(DELETE_ALL_BUT_LAST_BLOCK) where it used to be called.

Moved deletion to the destructor.

http://codereview.chromium.org/10534139/diff/1/src/zone.cc#newcode134
src/zone.cc:134: void Zone::DeleteAllButOne() {
On 2012/06/14 14:22:19, danno wrote:
> Just add a enum flag to DeleteAll that handles the last block specially.

Moved deletion to the destructor.

http://codereview.chromium.org/10534139/diff/1/src/zone.h
File src/zone.h (right):

http://codereview.chromium.org/10534139/diff/1/src/zone.h#newcode78
src/zone.h:78: void Destroy();
On 2012/06/14 14:22:19, danno wrote:
> Why not just make sure that the destructor of the Zone cleans up everything?
In
> the isolate case, the destructor never gets called and you will always keep
> around the extra small segment.

Yes, that is much cleaner.  Done.

[danno, 2012-06-19 15:24:57]

https://chromiumcodereview.appspot.com/10534139/diff/11001/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/10534139/diff/11001/src/runtime.cc#new...
src/runtime.cc:11205: CompilationInfo info(shared_info, &zone);
Ass discussed, move zone and zonescope into CompilationInfo

[sanjoy, 2012-06-19 16:46:50]

1. Added a subclass to CompilationInfo -- ZoneCompilationInfo which has its own
Zone and ZoneScope.  CompilationInfo (with an explicit Zone) is used only in
hydrogen.cc.

2. Fixed an issue which I had overlooked earlier: the FullCodeGenerator
constructor, instead of taking a  compilationInfo _and_ a Zone now takes only a
CompilationInfo since passing it both doesn't make sense.

[danno, 2012-06-19 20:09:53]

http://codereview.chromium.org/10534139/diff/16001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10534139/diff/16001/src/compiler.h#newcode268
src/compiler.h:268: class ZoneCompilationInfo: public CompilationInfo {
Please call this CompilationInfoWithZone, since both actually create everything
in a zone, and the difference is that this once has its own zone.

http://codereview.chromium.org/10534139/diff/16001/src/liveedit.cc
File src/liveedit.cc (right):

http://codereview.chromium.org/10534139/diff/16001/src/liveedit.cc#newcode896
src/liveedit.cc:896: Isolate* isolate = Isolate::Current();
Why again is it OK to remove the zone scope? Seems necessary...

[sanjoy, 2012-06-19 20:56:40]

http://codereview.chromium.org/10534139/diff/16001/src/compiler.h
File src/compiler.h (right):

http://codereview.chromium.org/10534139/diff/16001/src/compiler.h#newcode268
src/compiler.h:268: class ZoneCompilationInfo: public CompilationInfo {
On 2012/06/19 20:09:53, danno wrote:
> Please call this CompilationInfoWithZone, since both actually create
everything
> in a zone, and the difference is that this once has its own zone.

Done.

http://codereview.chromium.org/10534139/diff/16001/src/liveedit.cc
File src/liveedit.cc (right):

http://codereview.chromium.org/10534139/diff/16001/src/liveedit.cc#newcode896
src/liveedit.cc:896: Isolate* isolate = Isolate::Current();
On 2012/06/19 20:09:53, danno wrote:
> Why again is it OK to remove the zone scope? Seems necessary...

CompileScriptForTracker creates a CompilationInfoWithZone and hence a Zone and
ZoneScope.  This wasn't the case before.

[danno, 2012-06-20 08:57:43]

lgtm