Mark optimized code that used CHA for optimization so that lazy class 'finalization' does not inval…

Mark optimized code that used CHA for optimization so that lazy class 'finalization' does not invalidate that code.

R=fschneider@google.com

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

[Florian Schneider, 2014-01-31 12:24:41]

How about the approach we use for guarded fields? The optimizer / graph builder
registers functions that depend on a guarded field with the Field object.

Here for example, optimized functions that use CHA could be registered with the
Class object in a similar way.

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

https://codereview.chromium.org/150923002/diff/20001/runtime/vm/cha.cc#newcode95
runtime/vm/cha.cc:95: return false;
Can the set of direct subclasses change? If yes, probably need to set set the
flag here, too.

[srdjan, 2014-02-04 23:12:21]

PTAL

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

https://codereview.chromium.org/150923002/diff/20001/runtime/vm/cha.cc#newcode95
runtime/vm/cha.cc:95: return false;
On 2014/01/31 12:24:41, Florian Schneider wrote:
> Can the set of direct subclasses change? If yes, probably need to set set the
> flag here, too.

Thanks for catching it.

[Florian Schneider, 2014-02-05 10:36:30]

LGTM with comments.

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/isolate.h
File runtime/vm/isolate.h (right):

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:463: bool cha_used_;
The bool variables should be grouped together so that they occupy less space.

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2339: class WeakCodeReferences : public ValueObject {
This class and it's two subclasses seem good candidates for using a template
parameter instead of inheritance+abstract methods to make things more tidy:
Instead of 3 you have only two classes (for T=Field and T=Class) and there is no
unnecessary vtables.

template<classname T>
class WeakCodeReferences : public ValueObject {

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2341: explicit WeakCodeReferences(const Array& value) :
array_(value) {}
explicit WeakCodeReferences(const T& owner)
    : owner_(owner) { }

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2369: virtual void UpdateArrayTo(const Array& array) = 0;
void UpdateArrayTo(const Array& array) {
  owner_.set_cha_codes(array);
}

The other virtual methods can be written in the same manner.

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2430: const Array& array_;
Replace array_ with owner_.dependent_code() in all places and have

const T& owner_;

as the only member.

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2478: CHACodeArray a(*this);
WeakCodeReferences<Class> a(*this);

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2483: void Class::set_cha_codes(const Array& cache) const {
s/set_cha_codes/set_dependent_code_/g

s/cha_codes/dependent_code/g

so that Field and Class have a compatible interface to this template class.

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:6594: FieldDependentArray a(*this);
WeakCodeReferences<Field> a(*this);

[srdjan, 2014-02-05 16:20:51]

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2339: class WeakCodeReferences : public ValueObject {
On 2014/02/05 10:36:31, Florian Schneider wrote:
> This class and it's two subclasses seem good candidates for using a template
> parameter instead of inheritance+abstract methods to make things more tidy:
> Instead of 3 you have only two classes (for T=Field and T=Class) and there is
no
> unnecessary vtables.
> 
> template<classname T>
> class WeakCodeReferences : public ValueObject {

Templates are hard to work with: harder on compiler errors, harder to read,
harder to debug. They also create more code (larger footprint). Therefore I tend
to avoid them if possible. The overhead of the virtual call seems negligible in
this case.
Please reconsider the suggestion.

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2483: void Class::set_cha_codes(const Array& cache) const {
On 2014/02/05 10:36:31, Florian Schneider wrote:
> s/set_cha_codes/set_dependent_code_/g
> 
> s/cha_codes/dependent_code/g
> 
> so that Field and Class have a compatible interface to this template class.

Yes they would have 'compatible' interface but that design is fragile and
confusing since there is no 'dependency' relation.

[Florian Schneider, 2014-02-05 17:07:28]

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2339: class WeakCodeReferences : public ValueObject {
On 2014/02/05 16:20:51, srdjan wrote:
> On 2014/02/05 10:36:31, Florian Schneider wrote:
> > This class and it's two subclasses seem good candidates for using a template
> > parameter instead of inheritance+abstract methods to make things more tidy:
> > Instead of 3 you have only two classes (for T=Field and T=Class) and there
is
> no
> > unnecessary vtables.
> > 
> > template<classname T>
> > class WeakCodeReferences : public ValueObject {
> 
> Templates are hard to work with: harder on compiler errors, harder to read,
> harder to debug. They also create more code (larger footprint). Therefore I
tend
> to avoid them if possible. The overhead of the virtual call seems negligible
in
> this case.
> Please reconsider the suggestion.

I don't really care that much - so land it either way.

I prefer small tidy code over manully duplicated code. Many bugs arise from
manually copying code around.

Sure, the impact in this case is not noticable, but if there is a simple pattern
to avoid manual code duplication and the extra work of maintaining two copies of
the same code it, why not use it.

But ok, if you can avoid the code duplication otherwise (and without using a C
macro), I'm fine with that, too.

I just don't see the disadvantages to be relevant here: I don't buy the size
argument either, unless there is evidence that compilers will actually produce
bloated code here. I would not be surprised if the template version produces a
smaller binary.

You already have code duplication, or you can let the compiler duplicate the
code for you for the benefit of a cleaner, better maintainable source.

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2373: static bool IsOptimizedCode(const Array&
dependent_code, const Code& code) {
This checks for two things: the code is optimized and the code is in the array
of dependent code objects, so I'd rename it to:

s/IsOptimizedCode/IsDependentOptimizedCode/g

https://codereview.chromium.org/150923002/diff/250001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2483: void Class::set_cha_codes(const Array& cache) const {
On 2014/02/05 16:20:51, srdjan wrote:
> On 2014/02/05 10:36:31, Florian Schneider wrote:
> > s/set_cha_codes/set_dependent_code_/g
> > 
> > s/cha_codes/dependent_code/g
> > 
> > so that Field and Class have a compatible interface to this template class.
> 
> Yes they would have 'compatible' interface but that design is fragile and
> confusing since there is no 'dependency' relation.

Imo, fragile would imply that something bad could happen which is not the case:
you would just get a compile-time error about a unknown member.

[srdjan, 2014-02-05 20:32:10]

Committed patchset #6 manually as r32335 (presubmit successful).