Supported folding of constant size allocation followed by dynamic size allocation.

JSArray allocation is split into two separate allocations. Implemented folding of constant size allocation followed by dynamic size allocation.

[mvstanton, 2013-11-14 10:04:20]

Just a few comments, looking good!
--Michael

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc#oldcode2482
src/hydrogen.cc:2482: new_object->MakeDoubleAligned();
Why was this removed, is it handled during the folding step?

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc#newcode1925
src/hydrogen.cc:1925: int elements_size = IsFastDoubleElementsKind(kind)
I just ran into a similar situation. Constant-folding should take care of this,
so you don't need to manually optimize the code in this way. Have a look at the
generated hydrogen and you'll see that the "else" block here ends up as an
HConstant because it recognizes that the operands to HMul and HAdd are constant
themselves. You might have to teach HAdd::New and HMul::New to respect
ActualValue() to make it work.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc#newcode2448
src/hydrogen.cc:2448: // the JSArray allocation.
Nice.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc#newcode2480
src/hydrogen.cc:2480: 
Can you verify that a write barrier isn't used for the store of the elements
pointer? Previously, we were protected because elements_location was an
HInnerAllocatedObject. Now, we should be protected by the fact that both
allocations are NEW_SPACE allocations.

[Michael Starzinger, 2013-11-14 12:50:13]

First round of comments. The main comment is about kMaxNonCodeHeapObjectSize.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3390: HInstruction* current_instr =
HInstruction::cast(current_size);
The size might be an HPhi, in which case this cast will fail. We need to guard
against that.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3393: PrintF("#%d (%s) cannot fold into #%d (%s),
dynamic allocation size\n",
nit: The tracing text "dynamic allocation size" is ambiguous here.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3456: HInstruction* new_dominator_size =
HAdd::New(zone, context(),
Hmm, what happens of new_dominator_size gets bigger than
Page::kMaxNonCodeHeapObjectSize at runtime here. We definitely should dd a test
for that and then figure out a way to maintain the limit.

[Hannes Payer (out of office), 2013-11-21 12:07:19]

https://codereview.chromium.org/61463005/diff/360001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/61463005/diff/360001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3464: if (current_size->IsInteger32Constant()) {
The size check code with tracing output is almost the same, can we refactor
that?

https://codereview.chromium.org/61463005/diff/360001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/61463005/diff/360001/src/hydrogen.h#newcode1674
src/hydrogen.h:1674: // array must have been allocated with enough room for
Can you change "array" to "Array".

https://codereview.chromium.org/61463005/diff/360001/src/hydrogen.h#newcode1675
src/hydrogen.h:1675: // 1) the JSArray, 2) a AllocationMemento if mode requires
it.
-instead of "," -> "and"
-an AllocationMemento

https://codereview.chromium.org/61463005/diff/360001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/61463005/diff/360001/src/objects-printer.cc#n...
src/objects-printer.cc:952: CodeStub::MajorName(CodeStub::GetMajorKey(this),
true));
Why is this change in here?

[Igor Sheludko, 2013-11-27 12:52:00]

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3390: HInstruction* current_instr =
HInstruction::cast(current_size);
HPhi can't happen here, since the current_size is either constant size or a size
calculation graph (const_header_size + const_element_size * var_capacity). I'll
add assert and comment instead.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:3393: PrintF("#%d (%s) cannot fold into #%d (%s),
dynamic allocation size\n",
On 2013/11/14 12:50:13, Michael Starzinger wrote:
> nit: The tracing text "dynamic allocation size" is ambiguous here.

Done.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (left):

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc#oldcode2482
src/hydrogen.cc:2482: new_object->MakeDoubleAligned();
On 2013/11/14 10:04:20, mvstanton wrote:
> Why was this removed, is it handled during the folding step?

This flag is set in the constructor of the HAllocate for fixed array. And yes,
the folding step takes care of that.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc#newcode1925
src/hydrogen.cc:1925: int elements_size = IsFastDoubleElementsKind(kind)
On 2013/11/14 10:04:20, mvstanton wrote:
Ok, I removed manual constant folding code from here in this CL and in a
separate CL I'll make sure that constant folding works here as expected.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc#newcode2448
src/hydrogen.cc:2448: // the JSArray allocation.
On 2013/11/14 10:04:20, mvstanton wrote:
> Nice.

Done.

https://codereview.chromium.org/61463005/diff/20001/src/hydrogen.cc#newcode2480
src/hydrogen.cc:2480: 
If two allocations folds together then the second one becomes HInnerAllocation
with all the benefits as before. Unfortunately, if the allocations are not
folded then write barrier is NOT eliminated.

https://codereview.chromium.org/61463005/diff/360001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/61463005/diff/360001/src/hydrogen.h#newcode1674
src/hydrogen.h:1674: // array must have been allocated with enough room for
On 2013/11/21 12:07:19, Hannes Payer wrote:
> Can you change "array" to "Array".

Done.

https://codereview.chromium.org/61463005/diff/360001/src/hydrogen.h#newcode1675
src/hydrogen.h:1675: // 1) the JSArray, 2) a AllocationMemento if mode requires
it.
On 2013/11/21 12:07:19, Hannes Payer wrote:
> -instead of "," -> "and"
> -an AllocationMemento

Done.

https://codereview.chromium.org/61463005/diff/360001/src/objects-printer.cc
File src/objects-printer.cc (right):

https://codereview.chromium.org/61463005/diff/360001/src/objects-printer.cc#n...
src/objects-printer.cc:952: CodeStub::MajorName(CodeStub::GetMajorKey(this),
true));
Ok, I'll make a separate CL for that.

[Hannes Payer (out of office), 2013-11-29 09:14:28]

https://codereview.chromium.org/61463005/diff/540001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/61463005/diff/540001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3445:
!wired_allocate_->size()->IsInteger32Constant()) {
Can we not bail out here and check if the wired allocation fits?

A general comment: What about using the wired allocation concept to also get rid
of allocation mementos? For that, we would have to wire mementos to their
objects and make sure that the wired allocation also has enough space to get
folded.

We could then get rid of manual folding, in a separate cl.

[Hannes Payer (out of office), 2013-12-04 08:27:21]

https://codereview.chromium.org/61463005/diff/540001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/61463005/diff/540001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3443: // because otherwise it will prevent write
barrier optimization.
I just discussed the wired concept with Danno. We agreed on keeping the folding
code simple, i.e., don't wire allocations together, since it has no performance
impact.

[Igor Sheludko, 2013-12-30 19:20:48]

PTAL at new version.

https://codereview.chromium.org/61463005/diff/540001/src/hydrogen-instruction...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/61463005/diff/540001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3443: // because otherwise it will prevent write
barrier optimization.
On 2013/12/04 08:27:22, Hannes Payer wrote:
> I just discussed the wired concept with Danno. We agreed on keeping the
folding
> code simple, i.e., don't wire allocations together, since it has no
performance
> impact.
Ok, I'll revert wiring and implement explicit allocation size upper bound
propagation.

https://codereview.chromium.org/61463005/diff/540001/src/hydrogen-instruction...
src/hydrogen-instructions.cc:3445:
!wired_allocate_->size()->IsInteger32Constant()) {
On 2013/11/29 09:14:29, Hannes Payer wrote:
> Can we not bail out here and check if the wired allocation fits?
> 
> A general comment: What about using the wired allocation concept to also get
rid
> of allocation mementos? For that, we would have to wire mementos to their
> objects and make sure that the wired allocation also has enough space to get
> folded.
> 
> We could then get rid of manual folding, in a separate cl.
> 
>  
As we decided to revert wiring this note is no longer applicable.

[Hannes Payer (out of office), 2014-01-16 12:58:39]

Looking good, let's do another performance run.
LGTM

https://codereview.chromium.org/61463005/diff/610001/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/61463005/diff/610001/src/hydrogen-instruction...
src/hydrogen-instructions.h:5463: void set_size_upper_bound(HConstant* value) {
size_upper_bound_ = value; }
I guess set_size_upper_bound should only be called once for an instruction. Can
we assert that?

https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.cc#newcode2737
src/hydrogen.cc:2737: HConstant* elms_size_upper_bound =
capacity->IsInteger32Constant()
please use a full name elms -> elements

https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.cc#newcode2766
src/hydrogen.cc:2766: HValue* elms_size =
builder()->BuildCalculateElementsSize(kind_, capacity);
elms -> elements

https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.h
File src/hydrogen.h (right):

https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.h#newcode1693
src/hydrogen.h:1693: // Array must have been allocated with enough room for
Can you keep array lower case since it refers to the variable below

[Igor Sheludko, 2014-06-23 17:37:32]

On 2014/01/16 12:58:39, Hannes Payer wrote:
> Looking good, let's do another performance run.
> LGTM
> 
>
https://codereview.chromium.org/61463005/diff/610001/src/hydrogen-instructions.h
> File src/hydrogen-instructions.h (right):
> 
>
https://codereview.chromium.org/61463005/diff/610001/src/hydrogen-instruction...
> src/hydrogen-instructions.h:5463: void set_size_upper_bound(HConstant* value)
{
> size_upper_bound_ = value; }
> I guess set_size_upper_bound should only be called once for an instruction.
Can
> we assert that?
> 
> https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.cc
> File src/hydrogen.cc (right):
> 
>
https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.cc#newcode2737
> src/hydrogen.cc:2737: HConstant* elms_size_upper_bound =
> capacity->IsInteger32Constant()
> please use a full name elms -> elements
> 
>
https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.cc#newcode2766
> src/hydrogen.cc:2766: HValue* elms_size =
> builder()->BuildCalculateElementsSize(kind_, capacity);
> elms -> elements
> 
> https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.h
> File src/hydrogen.h (right):
> 
>
https://codereview.chromium.org/61463005/diff/610001/src/hydrogen.h#newcode1693
> src/hydrogen.h:1693: // Array must have been allocated with enough room for
> Can you keep array lower case since it refers to the variable below

Remake of this CL landed as https://codereview.chromium.org/304153009/