Do not eagerly finalize classes in CHA, instead regard unfinalized classes as ’non-existent’ an…

Do not eagerly finalize classes in CHA, instead regard unfinalized classes as ’non-existent’ and only invalidate optimized code at finalization of the class.
Rename FinalizePendingClasses to FinalizePendingClassInterfaces as the class finalization occurs lazily.
TODO: add dependency information to deoptimize/remove only relevant optimized code.

R=regis@google.com

Committed: https://code.google.com/p/dart/source/detail?r=30582

[regis, 2013-11-21 21:48:18]

LGTM

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/cha.cc#newcode89
runtime/vm/cha.cc:89: direct_subclass.LookupDynamicFunction(function_name) !=
missing parenthesis

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1538: frame = iterator.NextFrame();
How could this loop terminate before? Was it never executed?

https://codereview.chromium.org/81333003/diff/60003/tests/language/deopt_lazy...
File tests/language/deopt_lazy_finalization_test.dart (right):

https://codereview.chromium.org/81333003/diff/60003/tests/language/deopt_lazy...
tests/language/deopt_lazy_finalization_test.dart:19: return a.loop();
If there is a reason to call a.loop() twice, you should add a comment.

https://codereview.chromium.org/81333003/diff/60003/tests/language/deopt_lazy...
tests/language/deopt_lazy_finalization_test.dart:25: return b.loop();
ditto

[srdjan, 2013-11-21 22:25:00]

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/cha.cc#newcode89
runtime/vm/cha.cc:89: direct_subclass.LookupDynamicFunction(function_name) !=
On 2013/11/21 21:48:18, regis wrote:
> missing parenthesis

Done.

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/code_generator.cc
File runtime/vm/code_generator.cc (right):

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/code_generator...
runtime/vm/code_generator.cc:1538: frame = iterator.NextFrame();
On 2013/11/21 21:48:18, regis wrote:
> How could this loop terminate before? Was it never executed?

It was never executed. The code used to be called at lazy script loading, which
always occurred with no dart frame on stack.

https://codereview.chromium.org/81333003/diff/60003/tests/language/deopt_lazy...
File tests/language/deopt_lazy_finalization_test.dart (right):

https://codereview.chromium.org/81333003/diff/60003/tests/language/deopt_lazy...
tests/language/deopt_lazy_finalization_test.dart:19: return a.loop();
On 2013/11/21 21:48:18, regis wrote:
> If there is a reason to call a.loop() twice, you should add a comment.

Added comments. The reason is that only second time we call into optimized code
(OSR is off).

https://codereview.chromium.org/81333003/diff/60003/tests/language/deopt_lazy...
tests/language/deopt_lazy_finalization_test.dart:25: return b.loop();
On 2013/11/21 21:48:18, regis wrote:
> ditto

Done.

[Ivan Posva, 2013-11-21 23:04:09]

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/cha.cc#newcode80
runtime/vm/cha.cc:80: if (cls_direct_subclasses.IsNull()) {
We should initialize this with an empty array if necessary.

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:110: bool
ClassFinalizer::FinalizePendingClassInterfaces() {
This is not finalizing the class's interface. Actually we go to great pains to
not deal with the class' s interface (its methods and fields) here. What we are
finalizing at this point is the type hierarchy.

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:2075:
RemoveOptimizedCode(added_subclasses_to_cids);
This will add Object to the list, but Object never is a candidate for CHA as far
as I can tell.

[srdjan, 2013-11-22 00:08:14]

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/cha.cc
File runtime/vm/cha.cc (right):

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/cha.cc#newcode80
runtime/vm/cha.cc:80: if (cls_direct_subclasses.IsNull()) {
On 2013/11/21 23:04:09, Ivan Posva wrote:
> We should initialize this with an empty array if necessary.

direct_subclasses_ is of type GrowablObjectArray.

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/class_finalize...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:110: bool
ClassFinalizer::FinalizePendingClassInterfaces() {
On 2013/11/21 23:04:09, Ivan Posva wrote:
> This is not finalizing the class's interface. Actually we go to great pains to
> not deal with the class' s interface (its methods and fields) here. What we
are
> finalizing at this point is the type hierarchy.

Renaming to FinalizeTypeHierarchy

https://codereview.chromium.org/81333003/diff/60003/runtime/vm/class_finalize...
runtime/vm/class_finalizer.cc:2075:
RemoveOptimizedCode(added_subclasses_to_cids);
On 2013/11/21 23:04:09, Ivan Posva wrote:
> This will add Object to the list, but Object never is a candidate for CHA as
far
> as I can tell.

Modified CollectFinalizedSuperClasses to exclude object type.

[srdjan, 2013-11-22 17:59:42]

Committed patchset #9 manually as r30582 (presubmit successful).

[siva, 2013-11-22 18:09:53]

DBC

https://codereview.chromium.org/81333003/diff/880001/runtime/vm/class_finaliz...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/81333003/diff/880001/runtime/vm/class_finaliz...
runtime/vm/class_finalizer.cc:110: bool ClassFinalizer::FinalizeTypeHierarchy()
{
We collect all classes in a pending classes list
(object_store->pending_classes()) and this method was to process through this
pending_classes list. By renaming this to something that does not refer to
pending classes we lose that connection.

Can this be renamed to FinalizeTypeHierarchyOfPendingClasses or something like
that.