Track field types.

Track field types.

This is an initial step towards tracking the exact types instead of just the representations of fields. It adds support to track up to one map of heap object field values, eliminating various map checks on values loaded from such fields, at the cost of making stores to such fields slightly more expensive.

TEST=mjsunit/field-type-tracking
R=verwaest@chromium.org

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

[Toon Verwaest, 2014-03-04 13:49:34]

Overall the approach is looking good. Need more test cases and investigate why
performance drops. Possibly we need to avoid walking the entire transition tree
on each descriptor generalization.

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

https://codereview.chromium.org/167303005/diff/70001/src/flag-definitions.h#n...
src/flag-definitions.h:240: DEFINE_implication(track_field_types,
track_computed_fields)
I don't think you need computed_fields and double_fields.

https://codereview.chromium.org/167303005/diff/70001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/167303005/diff/70001/src/objects.cc#newcode2706
src/objects.cc:2706: next_descriptors->GetValue(i) != descriptors->GetValue(i))
break;
Please add a test for this case, ensuring that type feedback is properly merged.

https://codereview.chromium.org/167303005/diff/70001/src/objects.cc#newcode8208
src/objects.cc:8208: }
As stated above: Please test these cases.

[Toon Verwaest, 2014-03-21 14:33:37]

Looking good, but I still have some comments. Lets wait until after the branch.

https://codereview.chromium.org/167303005/diff/150001/src/hydrogen.cc
File src/hydrogen.cc (right):

https://codereview.chromium.org/167303005/diff/150001/src/hydrogen.cc#newcode...
src/hydrogen.cc:5489: field_map_->AddDependentCompilationInfo(
You really only need to add a dependency on these two things for loads, unless
you care about speeding up optimized code later once the field is generalized. I
presume the current choice is good enough, but we might need to revisit.

https://codereview.chromium.org/167303005/diff/150001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/167303005/diff/150001/src/objects.cc#newcode2028
src/objects.cc:2028: ? HeapType::OfCurrently(value, isolate)
If it's FORCE_TAGGED we also shouldn't be tracking types, right?
What about having a function that gets the representation in, in addition to the
object? That way, if the representation is tagged, we don't track?

https://codereview.chromium.org/167303005/diff/150001/src/objects.cc#newcode2753
src/objects.cc:2753: if (old_details.type() == FIELD) {
Only if new_representation is heap_object right?

https://codereview.chromium.org/167303005/diff/150001/src/objects.cc#newcode2818
src/objects.cc:2818: new_descriptors->SetValue(modify_index, *new_field_type);
Only if new_representation.IsHeapObject()

https://codereview.chromium.org/167303005/diff/150001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/167303005/diff/150001/src/objects.h#newcode1499
src/objects.h:1499: FLAG_track_heap_object_fields) && IsUninitialized()) {
Why did you add this flag? Which piece of code passes in uninitialized for
non-computed fields?

https://codereview.chromium.org/167303005/diff/150001/src/property.h
File src/property.h (right):

https://codereview.chromium.org/167303005/diff/150001/src/property.h#newcode472
src/property.h:472: return GetFieldTypeFromMap(holder()->map());
Aarg, this is ugly. We really need to cache holder()->map() rather than
holder(). Future CL I guess.

[Benedikt Meurer, 2014-04-11 11:11:59]

Please take another look...

https://codereview.chromium.org/167303005/diff/150001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/167303005/diff/150001/src/objects.cc#newcode2028
src/objects.cc:2028: ? HeapType::OfCurrently(value, isolate)
On 2014/03/21 14:33:37, Toon Verwaest wrote:
> If it's FORCE_TAGGED we also shouldn't be tracking types, right?
> What about having a function that gets the representation in, in addition to
the
> object? That way, if the representation is tagged, we don't track?

Done.

https://codereview.chromium.org/167303005/diff/150001/src/objects.cc#newcode2818
src/objects.cc:2818: new_descriptors->SetValue(modify_index, *new_field_type);
No, we track types independent of the representation.

https://codereview.chromium.org/167303005/diff/150001/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/167303005/diff/150001/src/objects.h#newcode1499
src/objects.h:1499: FLAG_track_heap_object_fields) && IsUninitialized()) {
This was a hack to work-around the "missing chokepoint for generalization"
issue. Fixed now.

https://codereview.chromium.org/167303005/diff/150001/src/property.h
File src/property.h (right):

https://codereview.chromium.org/167303005/diff/150001/src/property.h#newcode472
src/property.h:472: return GetFieldTypeFromMap(holder()->map());
ACK.

[Toon Verwaest, 2014-04-11 12:49:42]

lgtm with nits

https://codereview.chromium.org/167303005/diff/210001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode2022
src/objects.cc:2022: // Allocate new instance descriptors with (name, index)
added
This comment is a bit weird. I'd just drop it entirely; unless you feel strongly
about indicating that CopyAddFieldDescriptor copies something while adding a
field descriptor :)

https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode2849
src/objects.cc:2849: new_field_type = HeapType::Any(updated->GetIsolate());
Could you introduce a method similar to
new_representation.generalize(old_representation) for merging types? Would be
nice to not have this logic spread all over... (or at least, have an abstraction
for it).

https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode8213
src/objects.cc:8213: if (right_type->NowIs(left_type)) {
Same generalize logic would apply here

https://codereview.chromium.org/167303005/diff/210001/src/property.h
File src/property.h (right):

https://codereview.chromium.org/167303005/diff/210001/src/property.h#newcode202
src/property.h:202: return value->FitsRepresentation(representation());
return true;
Functions can take any value as argument.

[Benedikt Meurer, 2014-04-14 06:29:04]

https://codereview.chromium.org/167303005/diff/210001/src/objects.cc
File src/objects.cc (right):

https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode2022
src/objects.cc:2022: // Allocate new instance descriptors with (name, index)
added
On 2014/04/11 12:49:43, Toon Verwaest wrote:
> This comment is a bit weird. I'd just drop it entirely; unless you feel
strongly
> about indicating that CopyAddFieldDescriptor copies something while adding a
> field descriptor :)

Done.

https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode2849
src/objects.cc:2849: new_field_type = HeapType::Any(updated->GetIsolate());
On 2014/04/11 12:49:43, Toon Verwaest wrote:
> Could you introduce a method similar to
> new_representation.generalize(old_representation) for merging types? Would be
> nice to not have this logic spread all over... (or at least, have an
abstraction
> for it).

Done.

https://codereview.chromium.org/167303005/diff/210001/src/objects.cc#newcode8213
src/objects.cc:8213: if (right_type->NowIs(left_type)) {
On 2014/04/11 12:49:43, Toon Verwaest wrote:
> Same generalize logic would apply here

Done.

https://codereview.chromium.org/167303005/diff/210001/src/property.h
File src/property.h (right):

https://codereview.chromium.org/167303005/diff/210001/src/property.h#newcode202
src/property.h:202: return value->FitsRepresentation(representation());
On 2014/04/11 12:49:43, Toon Verwaest wrote:
> return true;
> Functions can take any value as argument.

Done.

[Benedikt Meurer, 2014-04-14 06:29:32]

Committed patchset #8 manually as r20701 (presubmit successful).