Refactoring to clean up duplicate code in Heap::Allocate methods.

Refactoring to clean up duplicate code in Heap::Allocate methods.

R=hpayer@chromium.org

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

[mvstanton, 2014-02-18 16:34:58]

Hi Hannes, here is the refactoring you asked about. Thanks for the suggestion, I
think it's a good improvement.

I moved some array-specific logic out of heap.cc, into runtime.cc
(ArrayConstructorCommon), where it belongs better.

PTAL,
--Michael

[Hannes Payer (out of office), 2014-02-19 21:29:06]

https://codereview.chromium.org/170703002/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode688
src/heap.h:688: // pretenure state.
What do you mean by "does not influence the pretenure state"?

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode693
src/heap.h:693: AllocationSite* allocation_site = NULL);
What about implementing an AllocationMode, similar to HAllocationMode?

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode693
src/heap.h:693: AllocationSite* allocation_site = NULL);
Why did you get rid of the Handle?

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode774
src/heap.h:774: // points to the site. It doesn't influence pretenure state.
same comments as above

https://codereview.chromium.org/170703002/diff/1/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/170703002/diff/1/src/runtime.cc#newcode14729
src/runtime.cc:14729: // don't emit a memento for them.
What about pretenuring in this case?

[Hannes Payer (out of office), 2014-02-20 18:15:53]

Did you delete the latest patch set?

[mvstanton, 2014-02-26 09:47:41]

Hi Hannes, no I didn't delete any patch set?
Here were my comments from last week.
In general, I kept the refactoring "small." See if that is okay with you and
then I could follow up with changes that bring something like HAllocationMode to
heap.h.
Thanks,
--Michael

https://codereview.chromium.org/170703002/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode688
src/heap.h:688: // pretenure state.
On 2014/02/19 21:29:07, Hannes Payer wrote:
> What do you mean by "does not influence the pretenure state"?

I mean that the AllocationSite is not mined for information used to determine
the Space for the allocation.

Perhaps there should be some renaming of these methods. The way they function is
that a site is passed only so that a memento can be allocated with the object.
Maybe the argument should be called "site_for_memento."

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode693
src/heap.h:693: AllocationSite* allocation_site = NULL);
On 2014/02/19 21:29:07, Hannes Payer wrote:
> Why did you get rid of the Handle?

Because it's not a handlefied method and the other arguments to the method are
pointers. The use of a handle was asymmetrical, and sends a contradictory
message to the "raw" MaybeObject* style of the method.

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode693
src/heap.h:693: AllocationSite* allocation_site = NULL);
On 2014/02/19 21:29:07, Hannes Payer wrote:
> What about implementing an AllocationMode, similar to HAllocationMode?

I am thinking small with this CL, focused on eliminating duplicate code rather
than introducing new constructs.

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode774
src/heap.h:774: // points to the site. It doesn't influence pretenure state.
On 2014/02/19 21:29:07, Hannes Payer wrote:
> same comments as above

Indeed. And probably the fact that I have to explain this (unsuccessfully at
that) implies that the API is broken anyway!

[Hannes Payer (out of office), 2014-03-03 17:03:45]

LGTM, just comment nits

https://codereview.chromium.org/170703002/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode688
src/heap.h:688: // pretenure state.
On 2014/02/26 09:47:41, mvstanton wrote:
> On 2014/02/19 21:29:07, Hannes Payer wrote:
> > What do you mean by "does not influence the pretenure state"?
> 
> I mean that the AllocationSite is not mined for information used to determine
> the Space for the allocation.
> 
> Perhaps there should be some renaming of these methods. The way they function
is
> that a site is passed only so that a memento can be allocated with the object.
> Maybe the argument should be called "site_for_memento."

Just drop the "allocation_site does not influence the pretenure state."

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode693
src/heap.h:693: AllocationSite* allocation_site = NULL);
On 2014/02/26 09:47:41, mvstanton wrote:
> On 2014/02/19 21:29:07, Hannes Payer wrote:
> > What about implementing an AllocationMode, similar to HAllocationMode?
> 
> I am thinking small with this CL, focused on eliminating duplicate code rather
> than introducing new constructs.

OK, I guess we should implement an AllocationMode in a follow-up CL.

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode774
src/heap.h:774: // points to the site. It doesn't influence pretenure state.
On 2014/02/26 09:47:41, mvstanton wrote:
> On 2014/02/19 21:29:07, Hannes Payer wrote:
> > same comments as above
> 
> Indeed. And probably the fact that I have to explain this (unsuccessfully at
> that) implies that the API is broken anyway!

Just drop the "It doesn't influence pretenure state."

[mvstanton, 2014-03-04 09:52:02]

thx Hannes, will check in when tree opens...

https://codereview.chromium.org/170703002/diff/1/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode688
src/heap.h:688: // pretenure state.
On 2014/03/03 17:03:46, Hannes Payer wrote:
> On 2014/02/26 09:47:41, mvstanton wrote:
> > On 2014/02/19 21:29:07, Hannes Payer wrote:
> > > What do you mean by "does not influence the pretenure state"?
> > 
> > I mean that the AllocationSite is not mined for information used to
determine
> > the Space for the allocation.
> > 
> > Perhaps there should be some renaming of these methods. The way they
function
> is
> > that a site is passed only so that a memento can be allocated with the
object.
> > Maybe the argument should be called "site_for_memento."
> 
> Just drop the "allocation_site does not influence the pretenure state."

Done.

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode693
src/heap.h:693: AllocationSite* allocation_site = NULL);
On 2014/03/03 17:03:46, Hannes Payer wrote:
> On 2014/02/26 09:47:41, mvstanton wrote:
> > On 2014/02/19 21:29:07, Hannes Payer wrote:
> > > What about implementing an AllocationMode, similar to HAllocationMode?
> > 
> > I am thinking small with this CL, focused on eliminating duplicate code
rather
> > than introducing new constructs.
> 
> OK, I guess we should implement an AllocationMode in a follow-up CL.

yep, thanks, will be done.

https://codereview.chromium.org/170703002/diff/1/src/heap.h#newcode774
src/heap.h:774: // points to the site. It doesn't influence pretenure state.
On 2014/03/03 17:03:46, Hannes Payer wrote:
> On 2014/02/26 09:47:41, mvstanton wrote:
> > On 2014/02/19 21:29:07, Hannes Payer wrote:
> > > same comments as above
> > 
> > Indeed. And probably the fact that I have to explain this (unsuccessfully at
> > that) implies that the API is broken anyway!
> 
> Just drop the "It doesn't influence pretenure state."

Done.

[mvstanton, 2014-03-04 12:51:46]

Committed patchset #2 manually as r19651 (presubmit successful).