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#newcode54 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.h#newcode57 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.h#newcode54 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#newcode56 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. http://codereview.chromium.org/10534139/ -- v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev
