Use mutable heapnumbers to store doubles in fields.

Use mutable heapnumbers to store doubles in fields.

R=danno@chromium.org

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

[Toon Verwaest, 2013-05-02 17:08:38]

PTAL

[danno, 2013-05-07 13:04:47]

Getting there... could you please add relevant JSON tests and double transition
tests of normal objects.

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

https://codereview.chromium.org/14850006/diff/42001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2525: if (map->is_deprecated()) continue;
Shouldn't be hitting this case? Maybe this should be an assert.

https://codereview.chromium.org/14850006/diff/42001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2538:
lookup.GetPropertyDetails().representation().IsDouble()) {
Do lookup.representation directly.

https://codereview.chromium.org/14850006/diff/42001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/ia32/stub-cache-ia32....
src/ia32/stub-cache-ia32.cc:910: __ mov(FieldOperand(receiver_reg, offset),
name_reg);
Give this name_reg guy another name?

https://codereview.chromium.org/14850006/diff/42001/src/ia32/stub-cache-ia32....
src/ia32/stub-cache-ia32.cc:1078: __ RecordWriteField(scratch1,
Remove the write barrier in the double case

https://codereview.chromium.org/14850006/diff/42001/src/json-parser.h
File src/json-parser.h (right):

https://codereview.chromium.org/14850006/diff/42001/src/json-parser.h#newcode432
src/json-parser.h:432: JSObject::TransitionToMap(json_object, map);
AllocateStorageForMap... better name?

https://codereview.chromium.org/14850006/diff/42001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/14850006/diff/42001/src/objects-inl.h#newcode290
src/objects-inl.h:290: MaybeObject* Object::StorageFor(Heap* heap,
Representation representation) {
AllocateNewStorageFor

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode824
src/objects.cc:824: return heap->AllocateHeapNumber(value->Number());
If this happens too frequently (i.e. having to allocate the HeapNumber after
calling FastPropertyAt, then shouldn't it actually be inside FastPropertyAt? And
have a FastPropertyStoreAtRaw access the unboxed value (it would be used less
frequently).

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode1719
src/objects.cc:1719: Representation representation) {
Perhaps a comment that this is a transition case. How about using the
AllocateNewStorageFor method?

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode1721
src/objects.cc:1721: if (FLAG_track_double_fields && representation.IsDouble())
{
Don't you just have to write into the existing box? This doesn't look like a
transition case.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode1800
src/objects.cc:1800: if (map()->unused_property_fields() == 0) {
Add a TODO to merge the second half of this method with AddFastPropertyUsingMap
(if possible). At least think about it.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode2291
src/objects.cc:2291: heap->AllocateHeapNumber(Smi::cast(value)->value(),
TENURED);
AllocateNewStorageFor

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode3921
src/objects.cc:3921: result = lookup.holder()->AddFastPropertyUsingMap(
Can you please revert to self?

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode5308
src/objects.cc:5308: MaybeObject* maybe_number =
heap->AllocateHeapNumber(value->Number());
NewStorageForValue

https://codereview.chromium.org/14850006/diff/42001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/parser.cc#newcode4022
src/parser.cc:4022: }
*COUGH* Any other better way to do this? *COUGH*
Boolean would be better.

https://codereview.chromium.org/14850006/diff/42001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/runtime.cc#newcode237
src/runtime.cc:237: Factory* factory = isolate->factory();
Don't need this change.

https://codereview.chromium.org/14850006/diff/42001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/stub-cache.cc#newcode228
src/stub-cache.cc:228: field.translate(receiver),
Extraneous change?

https://codereview.chromium.org/14850006/diff/42001/src/stub-cache.cc#newcode...
src/stub-cache.cc:1510: object, receiver(), holder, name, &miss);
Extraneous whitespace change

[Toon Verwaest, 2013-05-07 15:08:52]

Addressed comments, ptal again

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

https://codereview.chromium.org/14850006/diff/42001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2525: if (map->is_deprecated()) continue;
On 2013/05/07 13:04:47, danno wrote:
> Shouldn't be hitting this case? Maybe this should be an assert.

Done.

https://codereview.chromium.org/14850006/diff/42001/src/hydrogen-instructions...
src/hydrogen-instructions.cc:2538:
lookup.GetPropertyDetails().representation().IsDouble()) {
On 2013/05/07 13:04:47, danno wrote:
> Do lookup.representation directly.

Done.

https://codereview.chromium.org/14850006/diff/42001/src/ia32/stub-cache-ia32.cc
File src/ia32/stub-cache-ia32.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/ia32/stub-cache-ia32....
src/ia32/stub-cache-ia32.cc:910: __ mov(FieldOperand(receiver_reg, offset),
name_reg);
On 2013/05/07 13:04:47, danno wrote:
> Give this name_reg guy another name?

Done.

https://codereview.chromium.org/14850006/diff/42001/src/ia32/stub-cache-ia32....
src/ia32/stub-cache-ia32.cc:1078: __ RecordWriteField(scratch1,
On 2013/05/07 13:04:47, danno wrote:
> Remove the write barrier in the double case

Done. This was already done by returning in case of double. Cleaned up
unnecessary changes.

https://codereview.chromium.org/14850006/diff/42001/src/json-parser.h
File src/json-parser.h (right):

https://codereview.chromium.org/14850006/diff/42001/src/json-parser.h#newcode432
src/json-parser.h:432: JSObject::TransitionToMap(json_object, map);
On 2013/05/07 13:04:47, danno wrote:
> AllocateStorageForMap... better name?

Done.

https://codereview.chromium.org/14850006/diff/42001/src/objects-inl.h
File src/objects-inl.h (right):

https://codereview.chromium.org/14850006/diff/42001/src/objects-inl.h#newcode290
src/objects-inl.h:290: MaybeObject* Object::StorageFor(Heap* heap,
Representation representation) {
On 2013/05/07 13:04:47, danno wrote:
> AllocateNewStorageFor

Done.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode824
src/objects.cc:824: return heap->AllocateHeapNumber(value->Number());
On 2013/05/07 13:04:47, danno wrote:
> If this happens too frequently (i.e. having to allocate the HeapNumber after
> calling FastPropertyAt, then shouldn't it actually be inside FastPropertyAt?
And
> have a FastPropertyStoreAtRaw access the unboxed value (it would be used less
> frequently).

Done.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode1719
src/objects.cc:1719: Representation representation) {
On 2013/05/07 13:04:47, danno wrote:
> Perhaps a comment that this is a transition case. How about using the
> AllocateNewStorageFor method?

Done.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode1721
src/objects.cc:1721: if (FLAG_track_double_fields && representation.IsDouble())
{
It is the transition case; added comment.

On 2013/05/07 13:04:47, danno wrote:
> Don't you just have to write into the existing box? This doesn't look like a
> transition case.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode1800
src/objects.cc:1800: if (map()->unused_property_fields() == 0) {
On 2013/05/07 13:04:47, danno wrote:
> Add a TODO to merge the second half of this method with
AddFastPropertyUsingMap
> (if possible). At least think about it.

Done.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode2291
src/objects.cc:2291: heap->AllocateHeapNumber(Smi::cast(value)->value(),
TENURED);
On 2013/05/07 13:04:47, danno wrote:
> AllocateNewStorageFor

Done.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode3921
src/objects.cc:3921: result = lookup.holder()->AddFastPropertyUsingMap(
On 2013/05/07 13:04:47, danno wrote:
> Can you please revert to self?

Done.

https://codereview.chromium.org/14850006/diff/42001/src/objects.cc#newcode5308
src/objects.cc:5308: MaybeObject* maybe_number =
heap->AllocateHeapNumber(value->Number());
On 2013/05/07 13:04:47, danno wrote:
> NewStorageForValue

Done.

https://codereview.chromium.org/14850006/diff/42001/src/parser.cc
File src/parser.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/parser.cc#newcode4022
src/parser.cc:4022: }
On 2013/05/07 13:04:47, danno wrote:
> *COUGH* Any other better way to do this? *COUGH*
> Boolean would be better.

Done.

https://codereview.chromium.org/14850006/diff/42001/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/runtime.cc#newcode237
src/runtime.cc:237: Factory* factory = isolate->factory();
On 2013/05/07 13:04:47, danno wrote:
> Don't need this change.

Done.

https://codereview.chromium.org/14850006/diff/42001/src/stub-cache.cc
File src/stub-cache.cc (right):

https://codereview.chromium.org/14850006/diff/42001/src/stub-cache.cc#newcode228
src/stub-cache.cc:228: field.translate(receiver),
On 2013/05/07 13:04:47, danno wrote:
> Extraneous change?

Done.

https://codereview.chromium.org/14850006/diff/42001/src/stub-cache.cc#newcode...
src/stub-cache.cc:1510: object, receiver(), holder, name, &miss);
On 2013/05/07 13:04:47, danno wrote:
> Extraneous whitespace change

Done.

[danno, 2013-05-07 15:25:29]

ia32 looks fine with a comment. Please do the platform ports :-)

https://codereview.chromium.org/14850006/diff/50009/src/ia32/ic-ia32.cc
File src/ia32/ic-ia32.cc (right):

https://codereview.chromium.org/14850006/diff/50009/src/ia32/ic-ia32.cc#newco...
src/ia32/ic-ia32.cc:479: __ jmp(&slow);
You should remove this.

[Toon Verwaest, 2013-05-07 19:08:09]

Ported to ARM and X64. PTAL again.

[danno, 2013-05-08 14:59:11]

lgtm

[Toon Verwaest, 2013-05-08 15:02:33]

Committed patchset #4 manually as r14597 (presubmit successful).