Make embedded maps in optimized code weak.

Make embedded maps in optimized code weak.

Each map has a weak array of dependent codes, where the map tracks all the optimized codes that embed it. 
Old space GC either clears the dead dependent codes from the array if the corresponding map is alive or deoptimizes the live dependent codes if the map is dead.

BUG=v8:2073
R=mstarzinger@chromium.org

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

[ulan, 2012-12-13 14:37:25]

Please take a look. This depends on
https://chromiumcodereview.appspot.com/11547015/

https://chromiumcodereview.appspot.com/11575007/diff/3003/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/3003/src/objects.h#newco...
src/objects.h:4729: class DependentCodes {
I could make this a proper object that derives from FixedArray, but that seems
to be an overkill. WDYT?

[Michael Starzinger, 2012-12-14 14:15:26]

A first round of high-level comments.

https://codereview.chromium.org/11575007/diff/3003/src/objects-visiting-inl.h
File src/objects-visiting-inl.h (right):

https://codereview.chromium.org/11575007/diff/3003/src/objects-visiting-inl.h...
src/objects-visiting-inl.h:655: // Treat embedded maps in optimized code as
weak.
I think this is the wrong place to skip embedded maps because this visitor is
used in other places (e.g. HeapObject::IterateBody) as well. And there you don't
want to skip embedded maps. So this has to be specific to the
StaticMarkingVisitor. We already talked about this offline.

https://codereview.chromium.org/11575007/diff/3003/src/objects-visiting-inl.h...
src/objects-visiting-inl.h:699: // Treat embedded maps in optimized code as
weak.
Likewise.

https://codereview.chromium.org/11575007/diff/3003/src/objects.h
File src/objects.h (right):

https://codereview.chromium.org/11575007/diff/3003/src/objects.h#newcode4729
src/objects.h:4729: class DependentCodes {
On 2012/12/13 14:37:25, ulan wrote:
> I could make this a proper object that derives from FixedArray, but that seems
> to be an overkill. WDYT?

I think it should derive from FixedArray. One example is the TypeFeedbacksCells
class which does bascially the same. We don't need a separate instance type for
this though.

[ulan, 2013-01-17 09:35:10]

Please take another look.

https://chromiumcodereview.appspot.com/11575007/diff/3003/src/objects-visitin...
File src/objects-visiting-inl.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/3003/src/objects-visitin...
src/objects-visiting-inl.h:655: // Treat embedded maps in optimized code as
weak.
On 2012/12/14 14:15:26, Michael Starzinger wrote:
> I think this is the wrong place to skip embedded maps because this visitor is
> used in other places (e.g. HeapObject::IterateBody) as well. And there you
don't
> want to skip embedded maps. So this has to be specific to the
> StaticMarkingVisitor. We already talked about this offline.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/3003/src/objects-visitin...
src/objects-visiting-inl.h:699: // Treat embedded maps in optimized code as
weak.
On 2012/12/14 14:15:26, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/3003/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/3003/src/objects.h#newco...
src/objects.h:4729: class DependentCodes {
On 2012/12/14 14:15:26, Michael Starzinger wrote:
> On 2012/12/13 14:37:25, ulan wrote:
> > I could make this a proper object that derives from FixedArray, but that
seems
> > to be an overkill. WDYT?
> 
> I think it should derive from FixedArray. One example is the
TypeFeedbacksCells
> class which does bascially the same. We don't need a separate instance type
for
> this though.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
File src/objects-visiting-inl.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
src/objects-visiting-inl.h:696: if (map_visit_mode == SKIP_EMBEDDED_MAPS &&
kind() == OPTIMIZED_FUNCTION) {
Maybe I should just move this to VisitEmbeddedPointer of StaticMarkingVisitor?
This would require additional check for host()->kind() for each embedded pointer
though.

[Michael Starzinger, 2013-01-21 10:31:48]

Final round of reviews, after that we should be good to go.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
File src/objects-visiting-inl.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
src/objects-visiting-inl.h:395: // stack, this will make references from it
weak. We will clean dead
One "dead" oughta be enough. :)

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
src/objects-visiting-inl.h:397: Object** slot =
HeapObject::RawField(reinterpret_cast<HeapObject*>(map),
The reinterpret_cast shouldn't be necessary, a Map is already a HeapObject.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
src/objects-visiting-inl.h:696: if (map_visit_mode == SKIP_EMBEDDED_MAPS &&
kind() == OPTIMIZED_FUNCTION) {
On 2013/01/17 09:35:10, ulan wrote:
> Maybe I should just move this to VisitEmbeddedPointer of StaticMarkingVisitor?
> This would require additional check for host()->kind() for each embedded
pointer
> though.

Yes, good idea, I am very much in favor of this suggestion. We should see if it
has measurable performance impact and if not, then go for it.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.cc#new...
src/objects.cc:9708: }
This resizing is already implemented in Factory::CopySizeFixedArray, if you
change your sentinel value from Smi(0) to undefined you could reuse all of that
code. I'll leave that up to you.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.h#newc...
src/objects.h:4743: static const int kNumberOfCodesOffset = 0;
This should actually be called kNumberOfCodesIndex, because it is an index into
the array.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.h#newc...
src/objects.h:4744: static const int kCodesOffset = 1;
Likewise.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.h#newc...
src/objects.h:5181: bool CanTransition() {
I like this!

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/lithium.h
File src/lithium.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/lithium.h#newc...
src/lithium.h:701: void RegisterDependentCodeForEmbeddedMaps(Handle<Code> code);
Let's make this function protected instead of private.

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/mark-compact.c...
src/mark-compact.cc:85: 
Drop one of the empty newlines.

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/mark-compact.c...
src/mark-compact.cc:2189: void MarkCompactCollector::ClearNonLiveTransitions() {
This function should be renamed to "ClearNonLiveReferences" now that it deals
with more than transitions.

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/mark-compact.c...
src/mark-compact.cc:2217: DeoptimizeLiveDependentCodes(map);
How about naming this ClearAndDeoptimizeDependentCodes?

[Michael Starzinger, 2013-01-21 10:49:46]

One more comment.

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/lithium.cc
File src/lithium.cc (right):

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/lithium.cc#new...
src/lithium.cc:432: RegisterDependentCodeForEmbeddedMaps(code);
Let's move the call to this function into LCodeGen::FinishCode, I think it
belongs there.

[ulan, 2013-01-21 13:11:57]

Uploaded new patch set.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
File src/objects-visiting-inl.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
src/objects-visiting-inl.h:395: // stack, this will make references from it
weak. We will clean dead
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> One "dead" oughta be enough. :)

Done.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
src/objects-visiting-inl.h:397: Object** slot =
HeapObject::RawField(reinterpret_cast<HeapObject*>(map),
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> The reinterpret_cast shouldn't be necessary, a Map is already a HeapObject.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects-visiti...
src/objects-visiting-inl.h:696: if (map_visit_mode == SKIP_EMBEDDED_MAPS &&
kind() == OPTIMIZED_FUNCTION) {
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> On 2013/01/17 09:35:10, ulan wrote:
> > Maybe I should just move this to VisitEmbeddedPointer of
StaticMarkingVisitor?
> > This would require additional check for host()->kind() for each embedded
> pointer
> > though.
> 
> Yes, good idea, I am very much in favor of this suggestion. We should see if
it
> has measurable performance impact and if not, then go for it.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.cc
File src/objects.cc (right):

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.cc#new...
src/objects.cc:9708: }
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> This resizing is already implemented in Factory::CopySizeFixedArray, if you
> change your sentinel value from Smi(0) to undefined you could reuse all of
that
> code. I'll leave that up to you.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.h#newc...
src/objects.h:4743: static const int kNumberOfCodesOffset = 0;
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> This should actually be called kNumberOfCodesIndex, because it is an index
into
> the array.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/19001/src/objects.h#newc...
src/objects.h:4744: static const int kCodesOffset = 1;
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> Likewise.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/lithium.cc
File src/lithium.cc (right):

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/lithium.cc#new...
src/lithium.cc:432: RegisterDependentCodeForEmbeddedMaps(code);
On 2013/01/21 10:49:46, Michael Starzinger wrote:
> Let's move the call to this function into LCodeGen::FinishCode, I think it
> belongs there.
As discussed offline, leaving this here, otherwise we'd have to duplicate it for
each platform.

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/lithium.h
File src/lithium.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/lithium.h#newc...
src/lithium.h:701: void RegisterDependentCodeForEmbeddedMaps(Handle<Code> code);
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> Let's make this function protected instead of private.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/mark-compact.cc
File src/mark-compact.cc (right):

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/mark-compact.c...
src/mark-compact.cc:2189: void MarkCompactCollector::ClearNonLiveTransitions() {
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> This function should be renamed to "ClearNonLiveReferences" now that it deals
> with more than transitions.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/25001/src/mark-compact.c...
src/mark-compact.cc:2217: DeoptimizeLiveDependentCodes(map);
On 2013/01/21 10:31:48, Michael Starzinger wrote:
> How about naming this ClearAndDeoptimizeDependentCodes?

Done.

[Michael Starzinger, 2013-01-21 14:36:44]

LGTM (with a few final comments).

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
File src/objects-visiting-inl.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
src/objects-visiting-inl.h:178: if (rinfo->host()->kind() !=
Code::OPTIMIZED_FUNCTION ||
This should be behind the FLAG_collect_maps flag I guess, because we only force
deoptimization with this flag.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
src/objects-visiting-inl.h:649: // There are two places where we iterate code
bodies: here and the non-
s/non-templated/templated/

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
src/objects-visiting-inl.h:650: // templated CodeIterateBody (above). They
should be kept in sync.
s/above/below/

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
src/objects-visiting-inl.h:663: template<typename StaticVisitor,
Code::EmbeddedMapVisitMode map_visit_mode>
We should no longer need the template parameter.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects.h#newc...
src/objects.h:4521: enum EmbeddedMapVisitMode { VISIT_EMBEDDED_MAPS,
SKIP_EMBEDDED_MAPS };
We should no longer need this enum or the template parameter below.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects.h#newc...
src/objects.h:4620: STATIC_ASSERT(kMarkedForDeoptimizationBitCount +
This should be "kMarkedForDeoptimizationFirstBit" here.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects.h#newc...
src/objects.h:4684: // Smi(0) if the number of codes is less than the length of
the array.
Comment no longer applies, it's undefined instead of Smi(0) now.

[ulan, 2013-01-21 15:56:02]

Thank you for the feedback! I uploaded new patch set.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
File src/objects-visiting-inl.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
src/objects-visiting-inl.h:178: if (rinfo->host()->kind() !=
Code::OPTIMIZED_FUNCTION ||
On 2013/01/21 14:36:44, Michael Starzinger wrote:
> This should be behind the FLAG_collect_maps flag I guess, because we only
force
> deoptimization with this flag.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
src/objects-visiting-inl.h:649: // There are two places where we iterate code
bodies: here and the non-
On 2013/01/21 14:36:44, Michael Starzinger wrote:
> s/non-templated/templated/

Done.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
src/objects-visiting-inl.h:650: // templated CodeIterateBody (above). They
should be kept in sync.
On 2013/01/21 14:36:44, Michael Starzinger wrote:
> s/above/below/

Done.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects-visiti...
src/objects-visiting-inl.h:663: template<typename StaticVisitor,
Code::EmbeddedMapVisitMode map_visit_mode>
On 2013/01/21 14:36:44, Michael Starzinger wrote:
> We should no longer need the template parameter.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects.h
File src/objects.h (right):

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects.h#newc...
src/objects.h:4521: enum EmbeddedMapVisitMode { VISIT_EMBEDDED_MAPS,
SKIP_EMBEDDED_MAPS };
On 2013/01/21 14:36:44, Michael Starzinger wrote:
> We should no longer need this enum or the template parameter below.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects.h#newc...
src/objects.h:4620: STATIC_ASSERT(kMarkedForDeoptimizationBitCount +
On 2013/01/21 14:36:44, Michael Starzinger wrote:
> This should be "kMarkedForDeoptimizationFirstBit" here.

Done.

https://chromiumcodereview.appspot.com/11575007/diff/32001/src/objects.h#newc...
src/objects.h:4684: // Smi(0) if the number of codes is less than the length of
the array.
On 2013/01/21 14:36:44, Michael Starzinger wrote:
> Comment no longer applies, it's undefined instead of Smi(0) now.

Done.