Track representations of fields

Track representations of fields

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

[Toon Verwaest, 2013-04-22 16:09:46]

PTAL.
- Only ia32 for now.
- Currently only smi (track_fields) is done.
- Double (track_double_fields) is in flight, hence disabled.
- In case of accessors (callbacks) mixed with fields, there's the possibility of
multiple transition tree rewrites. These cases should probably be filtered out
by bailing out? (CopyGeneralizeStorage).

[Toon Verwaest, 2013-04-23 17:01:31]

Addressed comments, PTAL again.

[danno, 2013-04-24 15:23:00]

First round as discussed.

https://codereview.chromium.org/14146005/diff/12001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14146005/diff/12001/src/objects.cc#newcode2052
src/objects.cc:2052: old_target->InvalidateTransitionTree();
nit: How about InvalidateMapTransitionTree()?

https://codereview.chromium.org/14146005/diff/33001/src/arm/stub-cache-arm.cc
File src/arm/stub-cache-arm.cc (right):

https://codereview.chromium.org/14146005/diff/33001/src/arm/stub-cache-arm.cc...
src/arm/stub-cache-arm.cc:2917: if (!map->is_invalid_transition()) {
how about reversing the sense of the predicate to is_valid to make reading this
easier? Or add a second version in addition to the is_invalid?

https://codereview.chromium.org/14146005/diff/33001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/14146005/diff/33001/src/flag-definitions.h#ne...
src/flag-definitions.h:194: DEFINE_bool(track_double_fields, true, "track fields
with double values")
These should be off by default and enabled in another CL

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

https://codereview.chromium.org/14146005/diff/33001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5550: // if (FLAG_track_double_fields && index == 1
&&
Remove commented code?

https://codereview.chromium.org/14146005/diff/33001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5578: // (!FLAG_track_double_fields ||
field_representation_ != DOUBLE) &&
Remove commented code.

https://codereview.chromium.org/14146005/diff/33001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/14146005/diff/33001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:2973: XMMRegister xmm =
ToDoubleRegister(instr->result());
rename this to "result"

https://codereview.chromium.org/14146005/diff/33001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:2983: // TODO(verwaest): Implement noSSE2
support.
Use LoadKeyed as an example how to do this.

https://codereview.chromium.org/14146005/diff/33001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4265: if (instr->value()->IsConstantOperand())
{
If it's a double, disable constant in lithium-ia32.cc

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

https://codereview.chromium.org/14146005/diff/33001/src/ia32/stub-cache-ia32....
src/ia32/stub-cache-ia32.cc:784: __ mov(scratch1, transition);
Perhaps create a macro-assembler function for this?

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode185
src/ic.cc:185: if (target->type() != Code::NORMAL) {
Can you please add a comment here why this compare is relevant?

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode517
src/ic.cc:517: if (receiver->map()->is_invalid_transition()) {
is_invalid_transition_target or is_invalid. is_deprecated?

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode1106
src/ic.cc:1106: MapHandleList maps;
Comment that this is special handling that is only needed until we have
polymorphic store ICs.

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode1214
src/ic.cc:1214: 
extraneous whitespace change

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode1508
src/ic.cc:1508: return lookup->CanStore(value);
CanHoldValue?

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

https://codereview.chromium.org/14146005/diff/33001/src/json-parser.h#newcode431
src/json-parser.h:431: map, field, value->RequiredRepresentation());
OptimalRepresentation

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

https://codereview.chromium.org/14146005/diff/33001/src/objects-inl.h#newcode61
src/objects-inl.h:61: int value = value_ << 1;
Comment why this is needed?

https://codereview.chromium.org/14146005/diff/33001/src/objects-inl.h#newcode...
src/objects-inl.h:3563: bool Map::CanTransitionBeInvalidated() {
CanBeDeprecated

https://codereview.chromium.org/14146005/diff/33001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/14146005/diff/33001/src/objects.h#newcode1069
src/objects.h:1069: inline bool FitsRepresentation(Representation
representation) {
Check for FLAG_track_double_fields

[danno, 2013-04-24 16:08:20]

more comments

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2172
src/objects.cc:2172: // If fields were added (or removed), rewrite the instance.
ASSERT(target_number_of_fileds >= NumberOfFields())

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2294
src/objects.cc:2294: MaybeObject* Map::CopyGeneralizeRepresentation(
CopyGeneralizeAllRepresentations

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2308
src/objects.cc:2308: if (is_invalid_transition()) return;
Stack overflow?

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2478
src/objects.cc:2478: int descriptor = split_map->NumberOfOwnDescriptors();
split_descriptors

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2488
src/objects.cc:2488: for (; descriptor < descriptors; descriptor++) {
maybe a separate loop variable?

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2495
src/objects.cc:2495: Handle<Map>(new_map);
Please try to test this with a crazy big object example

[Toon Verwaest, 2013-04-25 10:59:38]

Addressed first set of comments.

https://codereview.chromium.org/14146005/diff/33001/src/flag-definitions.h
File src/flag-definitions.h (right):

https://codereview.chromium.org/14146005/diff/33001/src/flag-definitions.h#ne...
src/flag-definitions.h:194: DEFINE_bool(track_double_fields, true, "track fields
with double values")
On 2013/04/24 15:23:00, danno wrote:
> These should be off by default and enabled in another CL

Done.

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

https://codereview.chromium.org/14146005/diff/33001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5550: // if (FLAG_track_double_fields && index == 1
&&
On 2013/04/24 15:23:00, danno wrote:
> Remove commented code?

Done.

https://codereview.chromium.org/14146005/diff/33001/src/hydrogen-instructions...
src/hydrogen-instructions.h:5578: // (!FLAG_track_double_fields ||
field_representation_ != DOUBLE) &&
On 2013/04/24 15:23:00, danno wrote:
> Remove commented code.

Done.

https://codereview.chromium.org/14146005/diff/33001/src/ia32/lithium-codegen-...
File src/ia32/lithium-codegen-ia32.cc (right):

https://codereview.chromium.org/14146005/diff/33001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:2973: XMMRegister xmm =
ToDoubleRegister(instr->result());
On 2013/04/24 15:23:00, danno wrote:
> rename this to "result"

Done.

https://codereview.chromium.org/14146005/diff/33001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:2983: // TODO(verwaest): Implement noSSE2
support.
On 2013/04/24 15:23:00, danno wrote:
> Use LoadKeyed as an example how to do this.

Done.

https://codereview.chromium.org/14146005/diff/33001/src/ia32/lithium-codegen-...
src/ia32/lithium-codegen-ia32.cc:4265: if (instr->value()->IsConstantOperand())
{
On 2013/04/24 15:23:00, danno wrote:
> If it's a double, disable constant in lithium-ia32.cc 

Done.

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc
File src/ic.cc (right):

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode185
src/ic.cc:185: if (target->type() != Code::NORMAL) {
On 2013/04/24 15:23:00, danno wrote:
> Can you please add a comment here why this compare is relevant?

Done.

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode517
src/ic.cc:517: if (receiver->map()->is_invalid_transition()) {
On 2013/04/24 15:23:00, danno wrote:
> is_invalid_transition_target or is_invalid. is_deprecated?

Done.

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode1106
src/ic.cc:1106: MapHandleList maps;
On 2013/04/24 15:23:00, danno wrote:
> Comment that this is special handling that is only needed until we have
> polymorphic store ICs.

Done.

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode1214
src/ic.cc:1214: 
On 2013/04/24 15:23:00, danno wrote:
> extraneous whitespace change

Done.

https://codereview.chromium.org/14146005/diff/33001/src/ic.cc#newcode1508
src/ic.cc:1508: return lookup->CanStore(value);
On 2013/04/24 15:23:00, danno wrote:
> CanHoldValue?

Done.

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

https://codereview.chromium.org/14146005/diff/33001/src/json-parser.h#newcode431
src/json-parser.h:431: map, field, value->RequiredRepresentation());
On 2013/04/24 15:23:00, danno wrote:
> OptimalRepresentation

Done.

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

https://codereview.chromium.org/14146005/diff/33001/src/objects-inl.h#newcode61
src/objects-inl.h:61: int value = value_ << 1;
On 2013/04/24 15:23:00, danno wrote:
> Comment why this is needed?

Done.

https://codereview.chromium.org/14146005/diff/33001/src/objects-inl.h#newcode...
src/objects-inl.h:3563: bool Map::CanTransitionBeInvalidated() {
On 2013/04/24 15:23:00, danno wrote:
> CanBeDeprecated

Done.

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2172
src/objects.cc:2172: // If fields were added (or removed), rewrite the instance.
On 2013/04/24 16:08:20, danno wrote:
> ASSERT(target_number_of_fileds >= NumberOfFields())

Done.

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2294
src/objects.cc:2294: MaybeObject* Map::CopyGeneralizeRepresentation(
On 2013/04/24 16:08:20, danno wrote:
> CopyGeneralizeAllRepresentations

Done.

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2478
src/objects.cc:2478: int descriptor = split_map->NumberOfOwnDescriptors();
On 2013/04/24 16:08:20, danno wrote:
> split_descriptors

Done.

https://codereview.chromium.org/14146005/diff/33001/src/objects.cc#newcode2488
src/objects.cc:2488: for (; descriptor < descriptors; descriptor++) {
On 2013/04/24 16:08:20, danno wrote:
> maybe a separate loop variable?

Done.

https://codereview.chromium.org/14146005/diff/33001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/14146005/diff/33001/src/objects.h#newcode1069
src/objects.h:1069: inline bool FitsRepresentation(Representation
representation) {
On 2013/04/24 15:23:00, danno wrote:
> Check for FLAG_track_double_fields

Done.

[danno, 2013-04-25 13:33:48]

generally lgtm, but waiting for ARM port

https://codereview.chromium.org/14146005/diff/25/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/14146005/diff/25/src/objects.cc#newcode7289
src/objects.cc:7289: int current_offset = 0;
// 0 -> 'verbatim'

https://codereview.chromium.org/14146005/diff/25/src/objects.cc#newcode7296
src/objects.cc:7296: Name* key = GetKey(descriptor);
// |verbatim| -> |valid|

https://codereview.chromium.org/14146005/diff/25/src/objects.cc#newcode7302
src/objects.cc:7302: ASSERT(other_details.type() != CALLBACKS);
// Assert not correct?

https://codereview.chromium.org/14146005/diff/25/src/objects.cc#newcode7310
src/objects.cc:7310: result->Set(descriptor, &d, witness);
Merge this case with the previous one.

https://codereview.chromium.org/14146005/diff/25/src/objects.cc#newcode7339
src/objects.cc:7339: result->CopyFrom(descriptor, other, descriptor, witness);
Simplify logic as discussed. Either generate a field or copy, test should be
unified.

https://codereview.chromium.org/14146005/diff/25/src/objects.cc#newcode7343
src/objects.cc:7343: for (; descriptor < new_size; descriptor++) {
// |valid| -> new_size

https://codereview.chromium.org/14146005/diff/25/src/property-details.h
File src/property-details.h (right):

https://codereview.chromium.org/14146005/diff/25/src/property-details.h#newco...
src/property-details.h:132: uint8_t DetailsEncoded() {
Move this encoding to PropertyDetails

https://codereview.chromium.org/14146005/diff/25/src/property-details.h#newco...
src/property-details.h:183: PropertyDetails set_representation(Representation
representation) {
CopyForRepresentation

https://codereview.chromium.org/14146005/diff/25/src/runtime.cc
File src/runtime.cc (right):

https://codereview.chromium.org/14146005/diff/25/src/runtime.cc#newcode253
src/runtime.cc:253: // most from the beginning. However, since it may go slow by
adding
Elaborate, please :-) (fast)

https://codereview.chromium.org/14146005/diff/25/src/runtime.cc#newcode267
src/runtime.cc:267: // TODO(verwaest): Support tracking representations in the
boilerplate.
Remove all this code, insert TODO

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

https://codereview.chromium.org/14146005/diff/25/src/stub-cache.cc#newcode939
src/stub-cache.cc:939: Code::StubType type = handlers->at(0)->type();
Use number_of_valid_maps

[Toon Verwaest, 2013-04-25 14:49:10]

Addressed comments. Porting to ARM.

https://chromiumcodereview.appspot.com/14146005/diff/33001/src/ia32/stub-cach...
File src/ia32/stub-cache-ia32.cc (right):

https://chromiumcodereview.appspot.com/14146005/diff/33001/src/ia32/stub-cach...
src/ia32/stub-cache-ia32.cc:784: __ mov(scratch1, transition);
On 2013/04/24 15:23:00, danno wrote:
> Perhaps create a macro-assembler function for this?

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/14146005/diff/25/src/objects.cc#newcod...
src/objects.cc:7289: int current_offset = 0;
On 2013/04/25 13:33:48, danno wrote:
> // 0 -> 'verbatim'

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/objects.cc#newcod...
src/objects.cc:7296: Name* key = GetKey(descriptor);
On 2013/04/25 13:33:48, danno wrote:
> // |verbatim| -> |valid|

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/objects.cc#newcod...
src/objects.cc:7302: ASSERT(other_details.type() != CALLBACKS);
On 2013/04/25 13:33:48, danno wrote:
> // Assert not correct?

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/objects.cc#newcod...
src/objects.cc:7310: result->Set(descriptor, &d, witness);
On 2013/04/25 13:33:48, danno wrote:
> Merge this case with the previous one.

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/objects.cc#newcod...
src/objects.cc:7339: result->CopyFrom(descriptor, other, descriptor, witness);
On 2013/04/25 13:33:48, danno wrote:
> Simplify logic as discussed. Either generate a field or copy, test should be
> unified.

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/objects.cc#newcod...
src/objects.cc:7343: for (; descriptor < new_size; descriptor++) {
On 2013/04/25 13:33:48, danno wrote:
> // |valid| -> new_size

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/property-details.h
File src/property-details.h (right):

https://chromiumcodereview.appspot.com/14146005/diff/25/src/property-details....
src/property-details.h:132: uint8_t DetailsEncoded() {
On 2013/04/25 13:33:48, danno wrote:
> Move this encoding to PropertyDetails

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/property-details....
src/property-details.h:183: PropertyDetails set_representation(Representation
representation) {
On 2013/04/25 13:33:48, danno wrote:
> CopyForRepresentation

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/runtime.cc
File src/runtime.cc (right):

https://chromiumcodereview.appspot.com/14146005/diff/25/src/runtime.cc#newcod...
src/runtime.cc:253: // most from the beginning. However, since it may go slow by
adding
On 2013/04/25 13:33:48, danno wrote:
> Elaborate, please :-) (fast)

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/runtime.cc#newcod...
src/runtime.cc:267: // TODO(verwaest): Support tracking representations in the
boilerplate.
On 2013/04/25 13:33:48, danno wrote:
> Remove all this code, insert TODO

Done.

https://chromiumcodereview.appspot.com/14146005/diff/25/src/stub-cache.cc
File src/stub-cache.cc (right):

https://chromiumcodereview.appspot.com/14146005/diff/25/src/stub-cache.cc#new...
src/stub-cache.cc:939: Code::StubType type = handlers->at(0)->type();
On 2013/04/25 13:33:48, danno wrote:
> Use number_of_valid_maps

Done.

[Toon Verwaest, 2013-04-25 16:49:44]

Addressed comments and did ARM port. PTAL again.

[Toon Verwaest, 2013-04-26 15:32:02]

Committed patchset #11 manually as r14464 (presubmit successful).