Allocation Info Tracking, continued.

Allocation Info Tracking, continued.
"new Array()" cases.
Currently turned off.
This code puts necessary things in place.

BUG=

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

[mvstanton, 2013-01-24 15:18:02]

Hi Danno, I couldn't resist getting this stuff to work with --inline-new, this
will allow me to check out performance numbers for all the "new Array(...)"
cases.

More assembly programming that I've ever done :). No need to look at it now, but
if you are interested the meat is in builtins-ia32.cc.

AllocationMemento/Site is still pending, but to follow after some perf
investigations.

[mvstanton, 2013-02-13 08:51:08]

Hi guys, Toon, thx for doing the review...Danno, I think you'll have drive by
comments?

The ARM port currently has a bug, so give me until tomorrow on that piece.
Thanks,
--Michael

[Toon Verwaest, 2013-02-13 15:14:51]

First round of comments. Overall it looks good.

For the record (as discussed offline), in a future CL I'd like to see this
approach replaced by one that doesn't store magic numbers in the typefeedback
cells; but rather generate specific array constructors for different elements
kinds. Those array constructors point at their own fast-elements-kind map; which
in turn has the main Array function as constructor. The constructor stored in
the cell should only be internal and never leak to javascript, but allow us to
use the constructor directly rather than special case the constructor code.

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode212
src/builtins.cc:212: }
Remove { ... }; replace ToObject(& by To(&; set the type of initial_map as Map*;
so you can remove the Map::cast below.

May even be you don't need any of this code. The ArrayFunction should have an
initial map due to bootstrapping.

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode229
src/builtins.cc:229: if (holey) {
If this is holey, you'll probably want to update the type feedback. Otherwise
you'll create multiple objects with different maps, depending on the size. Eg:

function f(i) { return new Array(i); } will create holey and non-holey objects,
depending on the calls to f. f(0) becomes non-holey, and f(5) is holey. That's
bad for the ICs coming later on.

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode233
src/builtins.cc:233: if (IsMoreGeneralElementsKindTransition(kind, to_kind)) {
Shouldn't happen I guess. Otherwise we are doing a lot of work to track array
types; while we end up ignoring it anyway.

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode234
src/builtins.cc:234: maybe_array =
isolate->heap()->AllocateEmptyJSArray(to_kind);
Again; update type feedback in the cell.

https://codereview.chromium.org/11818021/diff/34001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/11818021/diff/34001/src/code-stubs.h#newcode1269
src/code-stubs.h:1269: int MinorKey() { return 0; }
I guess you don't need to overwrite the MinorKey?

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode1990
src/elements.cc:1990: {
Remove { ... }.

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode1997
src/elements.cc:1997: if (!maybe_obj->To(&fixed_array)) return maybe_obj;
It seems like this method should be merged with AllocateJSArrayAndStorage; if
possible. That can be done in a future CL though.

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode2014
src/elements.cc:2014: { MaybeObject* maybe_obj = array->Initialize(0);
You don't need the { .. }; and To(& rather than ToObject(&.

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode2023
src/elements.cc:2023: }
nit: I'd move this case above the == 1 case.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode3940
src/heap.cc:3940: { MaybeObject* maybe_result =
Remove {...}

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode3944
src/heap.cc:3944: if (!maybe_result->ToObject(&result)) return maybe_result;
->To(&

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4223
src/heap.cc:4223: {
Remove {. ->To(&.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4261
src/heap.cc:4261:
ASSERT((*allocation_site_info_payload)->IsJSGlobalPropertyCell());
The cast below will take care of this assert.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4264
src/heap.cc:4264: ASSERT(cell->value()->IsSmi());
The cast below will take care of this assert.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4270
src/heap.cc:4270: ASSERT(initial_map != NULL);
Yeah, it seems like you should ensure that the map is created if it isn't there
yet. Otherwise we'll end up with NULL-pointers as maps...

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4271
src/heap.cc:4271: // constructor->set_initial_map(Map::cast(initial_map));
omit Map::cast.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4273
src/heap.cc:4273: mode = DONT_TRACK_ALLOCATION_SITE;
Weird;... you do all this work to set up the initial state; but then you abort
and set the mode to DONT_TRACK?

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4317
src/heap.cc:4317: PrintF("Sorry, can't track yet in tenured space\n");
Euhm... Shouldn't we overwrite allocation_site_info_mode or so?

https://codereview.chromium.org/11818021/diff/34001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/11818021/diff/34001/src/heap.h#newcode606
src/heap.h:606: // didn't include it because I don't see why we need it yet.
Outdated comment?

https://codereview.chromium.org/11818021/diff/34001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/ia32/builtins-ia32.cc...
src/ia32/builtins-ia32.cc:1508: if (FLAG_optimize_constructed_arrays) {
Given that the 2 blocks of code are entirely independent, I'd split up this
function into 2 functions; and move the if to wherever this function is being
called.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:51: descriptor->stack_parameter_count_ = NULL;
Unnecessary move.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:61: static Register registers[] = { eax, ebx };
Please write a comment above identifying the purpose of these registers.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4856: __ j(equal, &done, Label::kFar);
I believe Label::kFar is the default, you can omit it if so.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4866: __ j(above, &miss, Label::kFar);
If you move this below the cmp, you can probably merge it with the equivalent
checks below.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4874: __ j(not_equal, &megamorphic_pre,
Label::kFar);
Jump to megamorphic directly.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4897: __ mov(ecx, FieldOperand(ecx,
GlobalObject::kGlobalContextOffset));
Check if this can be folded together with the code above.

https://codereview.chromium.org/11818021/diff/34001/src/isolate.cc
File src/isolate.cc (left):

https://codereview.chromium.org/11818021/diff/34001/src/isolate.cc#oldcode2080
src/isolate.cc:2080: deoptimizer_data_ = new DeoptimizerData;
Why did you move this?

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode9364
src/objects.cc:9364: }
Seems like this is overlapping with AllocateJSArrayAndStorage.

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10562
src/objects.cc:10562: MaybeObject* ret = NULL;
Seems like you don't need this variable. Below you can just return NULL; or
return payload->TransitionElementsKind(to_kind).

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10580
src/objects.cc:10580: if (length <= 8*1024) {
Define a constant.

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10599
src/objects.cc:10599: IsFastObjectElementsKind(to_kind);
This heuristic seems very buried away. Can we make it more top-level using some
function? Don't you already have that somewhere?

https://codereview.chromium.org/11818021/diff/34001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11818021/diff/34001/src/objects.h#newcode6986
src/objects.h:6986: 
Remove.

[mvstanton, 2013-02-19 11:04:08]

Hi Toon, thanks for the review, I've updated to address...

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc
File src/builtins.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode212
src/builtins.cc:212: }
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Remove { ... }; replace ToObject(& by To(&; set the type of initial_map as
Map*;
> so you can remove the Map::cast below.
> 
> May even be you don't need any of this code. The ArrayFunction should have an
> initial map due to bootstrapping.

Indeed, it won't be called until after the initial map is set, I could remove
it.

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode229
src/builtins.cc:229: if (holey) {
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> If this is holey, you'll probably want to update the type feedback. Otherwise
> you'll create multiple objects with different maps, depending on the size. Eg:
> 
> function f(i) { return new Array(i); } will create holey and non-holey
objects,
> depending on the calls to f. f(0) becomes non-holey, and f(5) is holey. That's
> bad for the ICs coming later on.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode233
src/builtins.cc:233: if (IsMoreGeneralElementsKindTransition(kind, to_kind)) {
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Shouldn't happen I guess. Otherwise we are doing a lot of work to track array
> types; while we end up ignoring it anyway.

good point, I'll let the ::GetMode(kind) call below decide if we continue
tracking allocation info and remove the if() block.

https://codereview.chromium.org/11818021/diff/34001/src/builtins.cc#newcode234
src/builtins.cc:234: maybe_array =
isolate->heap()->AllocateEmptyJSArray(to_kind);
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Again; update type feedback in the cell.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/code-stubs.h
File src/code-stubs.h (right):

https://codereview.chromium.org/11818021/diff/34001/src/code-stubs.h#newcode1269
src/code-stubs.h:1269: int MinorKey() { return 0; }
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> I guess you don't need to overwrite the MinorKey?

Actually you do, pure virtual...in the following checkin I will use it for
something.

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc
File src/elements.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode1990
src/elements.cc:1990: {
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Remove { ... }.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode1997
src/elements.cc:1997: if (!maybe_obj->To(&fixed_array)) return maybe_obj;
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> It seems like this method should be merged with AllocateJSArrayAndStorage; if
> possible. That can be done in a future CL though.

I did this, going through helper JSArray::Initialize(), which defers work to
AllocateJSArrayStorage().

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode2014
src/elements.cc:2014: { MaybeObject* maybe_obj = array->Initialize(0);
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> You don't need the { .. }; and To(& rather than ToObject(&.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/elements.cc#newcode2023
src/elements.cc:2023: }
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> nit: I'd move this case above the == 1 case.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode3940
src/heap.cc:3940: { MaybeObject* maybe_result =
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Remove {...}

Done, and I consolidated this function with the one above as my todo requested..

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4223
src/heap.cc:4223: {
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Remove {. ->To(&.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4223
src/heap.cc:4223: {
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Remove {. ->To(&.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4261
src/heap.cc:4261:
ASSERT((*allocation_site_info_payload)->IsJSGlobalPropertyCell());
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> The cast below will take care of this assert.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4264
src/heap.cc:4264: ASSERT(cell->value()->IsSmi());
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> The cast below will take care of this assert.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4270
src/heap.cc:4270: ASSERT(initial_map != NULL);
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Yeah, it seems like you should ensure that the map is created if it isn't
there
> yet. Otherwise we'll end up with NULL-pointers as maps...

Done.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4271
src/heap.cc:4271: // constructor->set_initial_map(Map::cast(initial_map));
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> omit Map::cast.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4273
src/heap.cc:4273: mode = DONT_TRACK_ALLOCATION_SITE;
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Weird;... you do all this work to set up the initial state; but then you abort
> and set the mode to DONT_TRACK?

The code is trying to express that the advice was consumed at this point, and
there is no need to allocate tracking information in the object now. It's
probably cleaner to leave that up to the caller to do, so I can cut this section
down.

https://codereview.chromium.org/11818021/diff/34001/src/heap.cc#newcode4317
src/heap.cc:4317: PrintF("Sorry, can't track yet in tenured space\n");
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Euhm... Shouldn't we overwrite allocation_site_info_mode or so?

I changed this to an assert that if you want pre-tenuring you can't also have
allocation tracking. That may be stronger than necessary, but I'd rather weaken
the conditions later if needed (by ignoring the request to track allocation
info).

https://codereview.chromium.org/11818021/diff/34001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/11818021/diff/34001/src/heap.h#newcode606
src/heap.h:606: // didn't include it because I don't see why we need it yet.
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Outdated comment?
yep, thx!

https://codereview.chromium.org/11818021/diff/34001/src/ia32/builtins-ia32.cc
File src/ia32/builtins-ia32.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/ia32/builtins-ia32.cc...
src/ia32/builtins-ia32.cc:1508: if (FLAG_optimize_constructed_arrays) {
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Given that the 2 blocks of code are entirely independent, I'd split up this
> function into 2 functions; and move the if to wherever this function is being
> called.

Actually I'd like to keep it this way because in the following checkin, the
FLAG_optimize_constructed_arrays will be removed...it just exists for this
checkin to hide the (not yet performant) feature. This function is exactly the
bottleneck for that feature.

Also, I would have to make a new built-in, because this function isn't called
directly, it's called under a kArrayConstructCode key.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32.cc
File src/ia32/code-stubs-ia32.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:61: static Register registers[] = { eax, ebx };
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Please write a comment above identifying the purpose of these registers.

I addressed this in the new function below,
InitializeArrayConstructorDescriptor().

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4856: __ j(equal, &done, Label::kFar);
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> I believe Label::kFar is the default, you can omit it if so.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4866: __ j(above, &miss, Label::kFar);
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> If you move this below the cmp, you can probably merge it with the equivalent
> checks below.

I looked more carefully and it didn't seem possible without burning another
register which I don't want to do. Instead I made a macro assembler method to do
this: LoadGlobalContext(reg).

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4874: __ j(not_equal, &megamorphic_pre,
Label::kFar);
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Jump to megamorphic directly.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/ia32/code-stubs-ia32....
src/ia32/code-stubs-ia32.cc:4897: __ mov(ecx, FieldOperand(ecx,
GlobalObject::kGlobalContextOffset));
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Check if this can be folded together with the code above.

Not foldable, but I have a new macro method: LoadGlobalContext(reg).

https://codereview.chromium.org/11818021/diff/34001/src/isolate.cc
File src/isolate.cc (left):

https://codereview.chromium.org/11818021/diff/34001/src/isolate.cc#oldcode2080
src/isolate.cc:2080: deoptimizer_data_ = new DeoptimizerData;
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Why did you move this?

Because the array constructor is a built-in, and now when the flag is enabled it
immediately deopts in the hydrogen stub.

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode9364
src/objects.cc:9364: }
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Seems like this is overlapping with AllocateJSArrayAndStorage.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10562
src/objects.cc:10562: MaybeObject* ret = NULL;
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Seems like you don't need this variable. Below you can just return NULL; or
> return payload->TransitionElementsKind(to_kind).

Done. And I addressed my TODO above, renaming the method.

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10580
src/objects.cc:10580: if (length <= 8*1024) {
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Define a constant.

Done.

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10599
src/objects.cc:10599: IsFastObjectElementsKind(to_kind);
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> This heuristic seems very buried away. Can we make it more top-level using
some
> function? Don't you already have that somewhere?

Yep, I just didn't revisit this code in a while, thx!

https://codereview.chromium.org/11818021/diff/34001/src/objects.cc#newcode10599
src/objects.cc:10599: IsFastObjectElementsKind(to_kind);
On 2013/02/13 15:14:51, Toon Verwaest wrote:
> This heuristic seems very buried away. Can we make it more top-level using
some
> function? Don't you already have that somewhere?

Done.

[Toon Verwaest, 2013-02-21 12:13:03]

Apart from some last nits, lgtm.

https://codereview.chromium.org/11818021/diff/34001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11818021/diff/34001/src/objects.h#newcode6986
src/objects.h:6986: 
... remove.

On 2013/02/13 15:14:51, Toon Verwaest wrote:
> Remove.

https://codereview.chromium.org/11818021/diff/52004/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/11818021/diff/52004/src/heap.cc#newcode4255
src/heap.cc:4255: // mode = DONT_TRACK_ALLOCATION_SITE;
Use GetMode(to_kind); here to decide whether or not to track ALLOCATION_SITE.

https://codereview.chromium.org/11818021/diff/52004/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/11818021/diff/52004/src/objects.cc#newcode10037
src/objects.cc:10037: if (trans->IsFailure()) return trans;
MaybeObject* maybe_failure = ...;
if (maybe_failure->IsFailure()) return maybe_failure;

https://codereview.chromium.org/11818021/diff/52004/src/objects.cc#newcode10053
src/objects.cc:10053: MaybeObject* trans = UpdateAllocationSiteInfo(kind);
MaybeObject* maybe_failure = ...;
if (maybe_failure->IsFailure()) return maybe_failure;

https://codereview.chromium.org/11818021/diff/52004/src/objects.cc#newcode10603
src/objects.cc:10603: return NULL;
return this;

https://codereview.chromium.org/11818021/diff/52004/src/objects.cc#newcode10608
src/objects.cc:10608: return NULL;
return this;

https://codereview.chromium.org/11818021/diff/52004/src/objects.cc#newcode10662
src/objects.cc:10662: if (trans->IsFailure()) return trans;
MaybeObject* maybe_failure = ...;
if (maybe_failure->IsFailure()) return maybe_failure;

[mvstanton, 2013-02-27 14:37:07]

Hi Toon,
Here are my comments from last week after your final review, along with new
changes for performance reasons.

The main changes are
1) Breaking up heap.cc functions into those that work with AllocationSiteINfo
and those that don't. I had performance issues in hot functions because of
introducing extra calls or adding parameters. So as much as possible, I made
separate functions.

2) Growing the builtins table does seem to hurt. For my following checkin I
don't need to grow it. Rather than plug in an ugly workaround I simply leave the
builtins table alone, and added a CHECK() that FLAG_optimize_constructed_arrays
is off. To turn that flag on, we have to have a larger builtins table. I'd
rather just deal with that in my next checkin.

3) Some code-sharing/refactoring ideas had to be left out because of the cost in
benchmarks. I do believe that these can be dealt with better in the following
checkin when performance gains due to hydrogen stubs will make it possible to
carry out these refactorings without regressions. I will include in that checkin
elimination of duplicate code due to the usual refactoring steps.

Thanks,
--Michael

ps - as I worked on performance I rebased several times, so I think I screwed
the pooch on the inter-revision view, sorry! :(

https://chromiumcodereview.appspot.com/11818021/diff/34001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11818021/diff/34001/src/objects.h#newc...
src/objects.h:6986: 
On 2013/02/21 12:13:03, Toon Verwaest wrote:
> ... remove.
> 
> On 2013/02/13 15:14:51, Toon Verwaest wrote:
> > Remove.
> 

Done.

https://chromiumcodereview.appspot.com/11818021/diff/52004/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11818021/diff/52004/src/heap.cc#newcod...
src/heap.cc:4255: // mode = DONT_TRACK_ALLOCATION_SITE;
On 2013/02/21 12:13:03, Toon Verwaest wrote:
> Use GetMode(to_kind); here to decide whether or not to track ALLOCATION_SITE.

Done.

https://chromiumcodereview.appspot.com/11818021/diff/52004/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/11818021/diff/52004/src/objects.cc#new...
src/objects.cc:10603: return NULL;
On 2013/02/21 12:13:03, Toon Verwaest wrote:
> return this;

Done.

https://chromiumcodereview.appspot.com/11818021/diff/52004/src/objects.cc#new...
src/objects.cc:10608: return NULL;
On 2013/02/21 12:13:03, Toon Verwaest wrote:
> return this;

Done.

https://chromiumcodereview.appspot.com/11818021/diff/52004/src/objects.cc#new...
src/objects.cc:10662: if (trans->IsFailure()) return trans;
On 2013/02/21 12:13:03, Toon Verwaest wrote:
> MaybeObject* maybe_failure = ...;
> if (maybe_failure->IsFailure()) return maybe_failure;

Done.

[Toon Verwaest, 2013-03-01 14:28:27]

Still lgtm.