ExtendStorageStub added, it is aimed for extending objects backing store when it runs out of space.

ExtendStorageStub added, it is aimed for extending objects backing store when it runs out of space.

R=yangguo@chromium.org

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

[Igor Sheludko, 2014-09-22 08:22:21]

ishell@chromium.org changed reviewers:
+ verwaest@chromium.org

[Igor Sheludko, 2014-09-22 08:22:21]

ishell@chromium.org changed reviewers:
+ verwaest@chromium.org

[Igor Sheludko, 2014-09-22 08:22:31]

PTAL

[Toon Verwaest, 2014-09-22 08:55:29]

I don't think you can reuse the Name register for ExtendStorage. What if you
deopt because of a mismatching representation?

Please add some tests for such cases.

https://codereview.chromium.org/587203002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/587203002/diff/1/src/hydrogen.cc#newcode2858
src/hydrogen.cc:2858: store->SetFlag(HValue::kAllowUndefinedAsNaN);
I don't think this flag has any meaning for FAST_ELEMENTS.

[Igor Sheludko, 2014-09-23 07:38:48]

I fixed the Name register issue in this patch set.

Yes, you are right, Name register must contain correct value in case of deopt. I
wasn't able to write the test that triggers deopt because mismatching
representation is handled in StoreTransition handler before the call to
ExtendStorageStub, but I tested that it works manually by temporarily inserting
a check that fails into the stub.

https://codereview.chromium.org/587203002/diff/1/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/587203002/diff/1/src/hydrogen.cc#newcode2858
src/hydrogen.cc:2858: store->SetFlag(HValue::kAllowUndefinedAsNaN);
On 2014/09/22 08:55:29, Toon Verwaest wrote:
> I don't think this flag has any meaning for FAST_ELEMENTS.

Done.

[Igor Sheludko, 2014-09-25 15:00:21]

ishell@chromium.org changed reviewers:
+ yangguo@chromium.org

[Igor Sheludko, 2014-09-25 15:00:22]

Yang, PTAL.

[Yang, 2014-09-29 11:10:38]

LGTM with comments.

https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:678:
BuildAllocateElementsAndInitializeElementsHeader(kind, new_capacity);
The name of that function is now somewhat misleading. We are allocating
properties, not elements. I don't have any idea how to rename it off the top of
my head though :/

https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:686:
BuildStoreNamedField(GetParameter(ExtendStorageDescriptor::kReceiverIndex),
Isn't this just 'object'?

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

https://codereview.chromium.org/587203002/diff/20001/src/code-stubs.h#newcode86
src/code-stubs.h:86: V(ExtendStorage)                          \
Put this before LoadConstant please. We try to maintain an alphabetical order.

[Igor Sheludko, 2014-09-29 12:33:11]

https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen.cc
File src/code-stubs-hydrogen.cc (right):

https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:678:
BuildAllocateElementsAndInitializeElementsHeader(kind, new_capacity);
On 2014/09/29 11:10:38, Yang wrote:
> The name of that function is now somewhat misleading. We are allocating
> properties, not elements. I don't have any idea how to rename it off the top
of
> my head though :/

Done.

https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen....
src/code-stubs-hydrogen.cc:686:
BuildStoreNamedField(GetParameter(ExtendStorageDescriptor::kReceiverIndex),
On 2014/09/29 11:10:38, Yang wrote:
> Isn't this just 'object'?

Done.

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

https://codereview.chromium.org/587203002/diff/20001/src/code-stubs.h#newcode86
src/code-stubs.h:86: V(ExtendStorage)                          \
On 2014/09/29 11:10:38, Yang wrote:
> Put this before LoadConstant please. We try to maintain an alphabetical order.

Done.

[Yang, 2014-09-29 12:41:03]

On 2014/09/29 12:33:11, Igor Sheludko wrote:
>
https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen.cc
> File src/code-stubs-hydrogen.cc (right):
> 
>
https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen....
> src/code-stubs-hydrogen.cc:678:
> BuildAllocateElementsAndInitializeElementsHeader(kind, new_capacity);
> On 2014/09/29 11:10:38, Yang wrote:
> > The name of that function is now somewhat misleading. We are allocating
> > properties, not elements. I don't have any idea how to rename it off the top
> of
> > my head though :/
> 
> Done.
> 
>
https://codereview.chromium.org/587203002/diff/20001/src/code-stubs-hydrogen....
> src/code-stubs-hydrogen.cc:686:
> BuildStoreNamedField(GetParameter(ExtendStorageDescriptor::kReceiverIndex),
> On 2014/09/29 11:10:38, Yang wrote:
> > Isn't this just 'object'?
> 
> Done.
> 
> https://codereview.chromium.org/587203002/diff/20001/src/code-stubs.h
> File src/code-stubs.h (right):
> 
>
https://codereview.chromium.org/587203002/diff/20001/src/code-stubs.h#newcode86
> src/code-stubs.h:86: V(ExtendStorage)                          \
> On 2014/09/29 11:10:38, Yang wrote:
> > Put this before LoadConstant please. We try to maintain an alphabetical
order.
> 
> Done.

lgtm.

[Igor Sheludko, 2014-09-29 13:11:50]

Committed patchset #3 (id:40001) manually as 24286 (presubmit successful).