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