Provide ability to patch external functions in a class that has already been finalized. The follow…

Provide ability to patch external functions in a class that has already been finalized.  The following restrictions are in place:
 - No new fields or patching of existing fields can be done
 - Only external methods that have not already executed (i.e code for it has been   generated) can be patched

This change enables patching of JS Interop methods into a class and provides close to 5 sec drop in start up times for Green Tea app.

R=hausner@google.com, regis@google.com

Committed: https://github.com/dart-lang/sdk/commit/78a6b4615557bd5634b8d0554e1c2fb47cc53ef7

[siva, 2016-03-31 17:35:32]

asiva@google.com changed reviewers:
+ hausner@google.com, regis@google.com

[regis, 2016-03-31 18:16:49]

LGTM with a few comments

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/class_finali...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/class_finali...
runtime/vm/class_finalizer.cc:2430: // or if the class is being refinalized
because a patch is being applied
or -> Or

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

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2834: "so far can be patched '%s'", name.ToCString());
The error message is not quite a correct sentence with the function name
appended.
You could make it clearer like this:
!orig_func.is_external() ? "'%s' is not external and therefore cannot be
patched" : "'%s' has already executed and therefore cannot be patched"

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2846: "only private new functions are allowed '%s'",
name.ToCString());
ditto
"'%s' is not private and therefore cannot be patched"

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:1274: void ResetFinalization() const;
The two calls SetRefinalize and ResetFinalization could easily be mixed up. A
few suggestions for more specific names:
SetRefinalizeAfterPatch
SetRefinalizePatched
SetPatchedUnfinalized
SetPostFinalizedPatched (to use your own term).

The state bit should be renamed accordingly.

[hausner, 2016-03-31 20:31:49]

LGTM with Q:

Is it necessary to introduce locks so the background compiler isn't attempting
to read the patched class while the mutator is updating the functions_ array? I
hope not.

[siva, 2016-03-31 23:38:29]

Description was changed from

==========
Provide ability to patch external functions in a class that has already been
finalized.  The following restrictions are in place:
 - No new fields or patching of existing fields can be done
 - Only external methods that have not already executed (i.e code for it has
been   generated) can be patched
==========

to

==========
Provide ability to patch external functions in a class that has already been
finalized.  The following restrictions are in place:
 - No new fields or patching of existing fields can be done
 - Only external methods that have not already executed (i.e code for it has
been   generated) can be patched

This change enables patching of JS Interop methods into a class and provides
close to 5 sec drop in start up times for Green Tea app.
==========

[siva, 2016-03-31 23:48:08]

The Class has been marked as not finalized (refinalize) so if the background
compiler attempts to compile anything it will first trigger EnsureIsFinalized
which aborts the background compiler.

I have also added an ASSERT that the patch application only happens on the
mutator thread.

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/class_finali...
File runtime/vm/class_finalizer.cc (right):

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/class_finali...
runtime/vm/class_finalizer.cc:2430: // or if the class is being refinalized
because a patch is being applied
On 2016/03/31 18:16:49, regis wrote:
> or -> Or

Done.

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

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2834: "so far can be patched '%s'", name.ToCString());
On 2016/03/31 18:16:49, regis wrote:
> The error message is not quite a correct sentence with the function name
> appended.
> You could make it clearer like this:
> !orig_func.is_external() ? "'%s' is not external and therefore cannot be
> patched" : "'%s' has already executed and therefore cannot be patched"

Done.

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:2846: "only private new functions are allowed '%s'",
name.ToCString());
On 2016/03/31 18:16:49, regis wrote:
> ditto
> "'%s' is not private and therefore cannot be patched"

Done.

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/object.h
File runtime/vm/object.h (right):

https://codereview.chromium.org/1850653003/diff/20001/runtime/vm/object.h#new...
runtime/vm/object.h:1274: void ResetFinalization() const;
On 2016/03/31 18:16:49, regis wrote:
> The two calls SetRefinalize and ResetFinalization could easily be mixed up. A
> few suggestions for more specific names:
> SetRefinalizeAfterPatch
> SetRefinalizePatched
> SetPatchedUnfinalized
> SetPostFinalizedPatched (to use your own term).
> 
> The state bit should be renamed accordingly.

I went with SetRefinalizeAfterPatch (a bit verbose but very clear).

[siva, 2016-04-01 03:06:48]

Description was changed from

==========
Provide ability to patch external functions in a class that has already been
finalized.  The following restrictions are in place:
 - No new fields or patching of existing fields can be done
 - Only external methods that have not already executed (i.e code for it has
been   generated) can be patched

This change enables patching of JS Interop methods into a class and provides
close to 5 sec drop in start up times for Green Tea app.
==========

to

==========
Provide ability to patch external functions in a class that has already been
finalized.  The following restrictions are in place:
 - No new fields or patching of existing fields can be done
 - Only external methods that have not already executed (i.e code for it has
been   generated) can be patched

This change enables patching of JS Interop methods into a class and provides
close to 5 sec drop in start up times for Green Tea app.

R=hausner@google.com, regis@google.com

Committed:
https://github.com/dart-lang/sdk/commit/78a6b4615557bd5634b8d0554e1c2fb47cc53ef7
==========

[siva, 2016-04-01 03:06:49]

Committed patchset #3 (id:40001) manually as
78a6b4615557bd5634b8d0554e1c2fb47cc53ef7 (presubmit successful).