- Support for patching of class methods and fields.

- Support for patching of class methods and fields.
Committed: https://code.google.com/p/dart/source/detail?r=10616

[Ivan Posva, 2012-08-14 00:37:31]

TBR=asiva,hausner

[Ivan Posva, 2012-08-14 01:21:49]

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.c...
runtime/vm/object.cc:1491: OS::PrintErr("patch_script: %s\n", str.ToCString());
Debug print removed...

[hausner, 2012-08-14 18:25:06]

I cannot LGTM this. I have two concerns: the runtime overhead of
function::Owner(), with is a commonly called function, and the impact the
external/patch hack has on the readability of the VM code, in particular the
parser.

I really hope we get rid of this hack and use the metadata concept instead.

https://chromiumcodereview.appspot.com/10827288/diff/11001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10827288/diff/11001/runtime/vm/object....
runtime/vm/object.cc:1487: void Class::ApplyPatch(const Class& with) const {
Would be nice to have a short comment on what this function does.

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.c...
runtime/vm/object.cc:4352: const Object& obj =
Object::Handle(raw_ptr()->owner_);
It would be nice if you could avoid this extra handle allocation (with implied
thread local storage lookup) for such a common operation, especially since it's
caused by this very uncommon corner case.

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.c...
runtime/vm/parser.cc:3025: // Do not install implicit constructors.
Why?

[siva, 2012-08-15 00:16:03]

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.c...
runtime/vm/object.cc:1504: const Array& new_functions =
Array::Handle(Array::New(patch_len + orig_len));
I think you should add a comment here that currently all patched functions are
prepended to the functions list and
maybe a TODO that this list will be changed to override the function in the
original list or append new functions if any.

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.c...
runtime/vm/object.cc:1520: patch_len = patch_fields.Length();
why not call the array handle variables orig_list, patch_list and new_list. You
could then resuse them between the function and field loop.

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.c...
runtime/vm/parser.cc:2963: class_name.ToCString());
I suppose it is an error to have multiple patch classes also right similar to
regular classes? i.e:

class A { .....} seen
patch class A { .... } seen
patch class A { ..... } this would be an error.

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.c...
runtime/vm/parser.cc:2975: }
There seems to be a duplication of this is already defined check, it was done
above in the if (!is_patch && ..) statement.

[Ivan Posva, 2012-08-15 06:50:39]

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.c...
runtime/vm/object.cc:1504: const Array& new_functions =
Array::Handle(Array::New(patch_len + orig_len));
On 2012/08/15 00:16:04, asiva wrote:
> I think you should add a comment here that currently all patched functions are
> prepended to the functions list and
> maybe a TODO that this list will be changed to override the function in the
> original list or append new functions if any.

I attempted to address the second part of your suggestion in the TODO on line
1501. I guess I failed. I will try to come up with a better wording in a
different CL unless I do implement the correct behaviour first.

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.c...
runtime/vm/object.cc:1520: patch_len = patch_fields.Length();
On 2012/08/15 00:16:04, asiva wrote:
> why not call the array handle variables orig_list, patch_list and new_list.
You
> could then resuse them between the function and field loop.
ditto

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/object.c...
runtime/vm/object.cc:4352: const Object& obj =
Object::Handle(raw_ptr()->owner_);
On 2012/08/14 18:25:06, hausner wrote:
> It would be nice if you could avoid this extra handle allocation (with implied
> thread local storage lookup) for such a common operation, especially since
it's
> caused by this very uncommon corner case.

I have seen 0% impact by this change on performance. It is just not called
enough when running the VM.

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.cc
File runtime/vm/parser.cc (right):

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.c...
runtime/vm/parser.cc:2963: class_name.ToCString());
On 2012/08/15 00:16:04, asiva wrote:
> I suppose it is an error to have multiple patch classes also right similar to
> regular classes? i.e:
> 
> class A { .....} seen
> patch class A { .... } seen
> patch class A { ..... } this would be an error.

Currently this error cannot be identified as I am not adding the patch class to
the library and thus I have no means of finding this error condition. I should
get to this when addressing the better error checking when merging the member
definitions in ApplyPatch.

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.c...
runtime/vm/parser.cc:2975: }
On 2012/08/15 00:16:04, asiva wrote:
> There seems to be a duplication of this is already defined check, it was done
> above in the if (!is_patch && ..) statement.

Merge-conflict resolution error. Thanks for noticing, fixing...

https://chromiumcodereview.appspot.com/10827288/diff/8004/runtime/vm/parser.c...
runtime/vm/parser.cc:3025: // Do not install implicit constructors.
On 2012/08/14 18:25:06, hausner wrote:
> Why? 

If a patch class does not patch a constructor, it should not be adding an
implicit constructor. I will split the CheckConstructors into two:
AddImplicitConstructor and CheckConstructors to not conflate the two things.