Initial isolate reload support

Initial isolate reload support

This is a cut of the work that Todd and I collaborated on in the reload branch.

In this CL, we've dropped the loader port hacks, in other words, on stack reloading in the standalone embedder does not work yet.

- [x] Support for hot reloading of isolate source code
- [x] Unit test harness and many tests
- [x] Service protocol and Observatory support
- [x] Product build does not include support for hot reloading.

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/48c8ffa7f3d778fb6ee39c558f7ffae9a134e7ad

[Cutch, 2016-05-10 16:46:12]

Description was changed from

==========
Isolate reload support
==========

to

==========
Initial isolate reload support

This is a cut of the work that Todd and I collaborated on.

In this CL, we've dropped the loader port hacks, in other words, on stack
reloading in the standalone embedder does not work yet. 

- [x] Support for hot reloading of isolate source code
- [x] Unit test harness and many tests
- [x] Service protocol and Observatory support
==========

[Cutch, 2016-05-10 16:46:12]

johnmccutchan@google.com changed reviewers:
+ iposva@google.com, rmacnak@google.com

[Cutch, 2016-05-10 16:46:52]

Description was changed from

==========
Initial isolate reload support

This is a cut of the work that Todd and I collaborated on.

In this CL, we've dropped the loader port hacks, in other words, on stack
reloading in the standalone embedder does not work yet. 

- [x] Support for hot reloading of isolate source code
- [x] Unit test harness and many tests
- [x] Service protocol and Observatory support
==========

to

==========
Initial isolate reload support

This is a cut of the work that Todd and I collaborated on.

In this CL, we've dropped the loader port hacks, in other words, on stack
reloading in the standalone embedder does not work yet. 

- [x] Support for hot reloading of isolate source code
- [x] Unit test harness and many tests
- [x] Service protocol and Observatory support
- [x] Product build does not include support for hot reloading.
==========

[Cutch, 2016-05-10 16:48:19]

Description was changed from

==========
Initial isolate reload support

This is a cut of the work that Todd and I collaborated on.

In this CL, we've dropped the loader port hacks, in other words, on stack
reloading in the standalone embedder does not work yet. 

- [x] Support for hot reloading of isolate source code
- [x] Unit test harness and many tests
- [x] Service protocol and Observatory support
- [x] Product build does not include support for hot reloading.
==========

to

==========
Initial isolate reload support

This is a cut of the work that Todd and I collaborated on in the reload branch.

In this CL, we've dropped the loader port hacks, in other words, on stack
reloading in the standalone embedder does not work yet. 

- [x] Support for hot reloading of isolate source code
- [x] Unit test harness and many tests
- [x] Service protocol and Observatory support
- [x] Product build does not include support for hot reloading.
==========

[rmacnak, 2016-05-11 19:56:19]

Some initial comments

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/include/...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/include/...
runtime/include/dart_api.h:2662: Dart_kScriptTag,
Update values in bin/builtin.dart?

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/observat...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/observat...
runtime/observatory/lib/src/elements/debugger.dart:1055:
IsolateReloadCommand(Debugger debugger) : super(debugger, 'reload', []);
General comment: I'm a bit uneasy about the name 'reload' because to me it means
the same thing as 'refresh'. If I 'reload' or 'refresh' a web page, I lose my
transient state.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/observat...
File runtime/observatory/lib/src/elements/isolate_view.html (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/observat...
runtime/observatory/lib/src/elements/isolate_view.html:101: label="reload
sources">
A brief description of reloading here would help.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/block...
File runtime/vm/block_scheduler.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/block...
runtime/vm/block_scheduler.cc:68: return;
ASSERT(at least one reload)

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
File runtime/vm/isolate_reload.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:157: const Library& a_lib =
Library::Handle(Class::Cast(a).library());
What classes don't have a library?

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:165: return a_lib_url.Equals(b_lib_url);
return IsSameLibrary(a_lib, b_lib)

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:242: Thread* thread = Thread::Current();
Add a comment that because we haven't yet decided if the change is good,
anything we do here must not change user visible state in case we have to abort
the change.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:245: const Library& root_lib =
Library::Handle(object_store()->root_library());
root_lib may be null if an isolate reload is requested before
Dart_LoadScript[FromSnapshot]

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:251: // assumptions from field guards or CHA.
or unloaded deferred prefixes

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:331: void
IsolateReloadContext::SwitchStackToUnoptimizedCode() {
Doesn't actually switch the function's code. Consider
EnsuredUnoptimizedCodeForStack.

It might also be possible the unoptimized code will be collected again before
the stack unwinds through all of these frames. Maybe reloading should require
--no-collect-code?

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:375: }
library prefixes also have dependent code.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:404: if (i != kFreeListElement) {
Why is this class special? I'd expect either all the predefined classes are
special or none are.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:908: static void ResetICs(const Function& function,
const Code& code) {
For code on the stack, we need to find the ICs in the object pool not in the ic
data array, as the ic data array may belong to a newer compilation than the Code
object.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:910: return;  // Already reset in an earlier round.
I'm wrong here. Even if the ic data array is already gone, we may still have
stale lookups in the ICs in the object pool.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:957: // referenced from the optimized code's deopt
object table, because this
This comment is out of date. Optimized code doesn't have a separate deopt object
table anymore; instead objects needed for deopt are include in the object pool.
Also an optimized code doesn't reference its corresponding unoptimized code
though this pool anymore, which is why we need to run EnsureUnoptimizedCode for
all the code on the stack.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:962: function = code.function();
code = function.unoptimized_code();
ASSERT(!code.IsNull());   // We did EnsureUnoptimizedCode in an earlier phase.
ResetICs(function, code);

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:1030: func.ZeroEdgeCounters();
Should only be needed in the PreserveUnoptimizedCode case.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:1059: // the sentinel values so that we have no
stale type feedback.
more importantly, no stale method lookups.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:1091: HeapIterationScope heap_iteration_scope;
ResetUnoptimizedICsOnStack isn't a heap walk. Move the scopes into
MarkAllFunctionsForRecompilation.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
File runtime/vm/isolate_reload_test.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload_test.cc:1767: #endif  // !PRODUCT
Missing cases with schema changes (add/remove fields or add/remove type
arguments), if only to test they are gracefully rejected for now instead of
producing some kind of crash.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/objec...
File runtime/vm/object_reload.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/objec...
runtime/vm/object_reload.cc:511: void ICData::Reset(bool is_static_call) const {
ICData should have an 'is_static_call' bit.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/raw_o...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/raw_o...
runtime/vm/raw_object.h:1734: RawObject* parameterized_class_id_;
Why not RawSmi* ?

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/servi...
File runtime/vm/service.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/servi...
runtime/vm/service.cc:2397: }
if (Dart::snapshot_kind() == kAppWithJIT) {
  "Cannot reload source when running an app snapshot"
}

[Cutch, 2016-05-11 20:09:28]

Ryan,

A lot of your comments are known TODO items that we plan on addressing after
this CL lands. The goal of this CL is not to call this work 'finished' but to
move our code off of our reload branch. To that point, we are primarily worried
about changes that would destabilize regular program execution.

Thanks,
John

[Cutch, 2016-05-12 15:50:14]

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/include/...
File runtime/include/dart_api.h (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/include/...
runtime/include/dart_api.h:2662: Dart_kScriptTag,
On 2016/05/11 19:56:17, rmacnak wrote:
> Update values in bin/builtin.dart?

kScriptTag is already defined in builtin.dart, also it looks like builtin.dart
references kResourceLoad which no longer exists.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/observat...
File runtime/observatory/lib/src/elements/debugger.dart (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/observat...
runtime/observatory/lib/src/elements/debugger.dart:1055:
IsolateReloadCommand(Debugger debugger) : super(debugger, 'reload', []);
On 2016/05/11 19:56:17, rmacnak wrote:
> General comment: I'm a bit uneasy about the name 'reload' because to me it
means
> the same thing as 'refresh'. If I 'reload' or 'refresh' a web page, I lose my
> transient state.

Acknowledged.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/observat...
File runtime/observatory/lib/src/elements/isolate_view.html (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/observat...
runtime/observatory/lib/src/elements/isolate_view.html:101: label="reload
sources">
On 2016/05/11 19:56:17, rmacnak wrote:
> A brief description of reloading here would help.

This is going to be removed before we submit.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/block...
File runtime/vm/block_scheduler.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/block...
runtime/vm/block_scheduler.cc:68: return;
On 2016/05/11 19:56:17, rmacnak wrote:
> ASSERT(at least one reload)

Done.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
File runtime/vm/isolate_reload.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:157: const Library& a_lib =
Library::Handle(Class::Cast(a).library());
On 2016/05/11 19:56:18, rmacnak wrote:
> What classes don't have a library?

Acknowledged.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:165: return a_lib_url.Equals(b_lib_url);
On 2016/05/11 19:56:18, rmacnak wrote:
> return IsSameLibrary(a_lib, b_lib)

Done.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:242: Thread* thread = Thread::Current();
On 2016/05/11 19:56:18, rmacnak wrote:
> Add a comment that because we haven't yet decided if the change is good,
> anything we do here must not change user visible state in case we have to
abort
> the change.

I don't think this is the right place for the comment. I've added a comment at
::Checkpoint().

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:245: const Library& root_lib =
Library::Handle(object_store()->root_library());
On 2016/05/11 19:56:18, rmacnak wrote:
> root_lib may be null if an isolate reload is requested before
> Dart_LoadScript[FromSnapshot]

We do not allow a reload to be triggered until the isolate is made runnable.
I've added an ASSERT here though.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:251: // assumptions from field guards or CHA.
On 2016/05/11 19:56:18, rmacnak wrote:
> or unloaded deferred prefixes

Done.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:331: void
IsolateReloadContext::SwitchStackToUnoptimizedCode() {
On 2016/05/11 19:56:18, rmacnak wrote:
> Doesn't actually switch the function's code. Consider
> EnsuredUnoptimizedCodeForStack.
> 

Done.

> It might also be possible the unoptimized code will be collected again before
> the stack unwinds through all of these frames. Maybe reloading should require
> --no-collect-code?

I've made it so we don't collect code if we've attempted a reload.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:375: }
On 2016/05/11 19:56:18, rmacnak wrote:
> library prefixes also have dependent code.

Added TODO

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:404: if (i != kFreeListElement) {
On 2016/05/11 19:56:18, rmacnak wrote:
> Why is this class special? I'd expect either all the predefined classes are
> special or none are.

It is special because it doesn't have a name and the set relies on the name.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:908: static void ResetICs(const Function& function,
const Code& code) {
On 2016/05/11 19:56:18, rmacnak wrote:
> For code on the stack, we need to find the ICs in the object pool not in the
ic
> data array, as the ic data array may belong to a newer compilation than the
Code
> object.

Added TODO.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:910: return;  // Already reset in an earlier round.
On 2016/05/11 19:56:18, rmacnak wrote:
> I'm wrong here. Even if the ic data array is already gone, we may still have
> stale lookups in the ICs in the object pool.

Acknowledged.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:957: // referenced from the optimized code's deopt
object table, because this
On 2016/05/11 19:56:18, rmacnak wrote:
> This comment is out of date. Optimized code doesn't have a separate deopt
object
> table anymore; instead objects needed for deopt are include in the object
pool.
> Also an optimized code doesn't reference its corresponding unoptimized code
> though this pool anymore, which is why we need to run EnsureUnoptimizedCode
for
> all the code on the stack.

Acknowledged.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:962: function = code.function();
On 2016/05/11 19:56:18, rmacnak wrote:
> code = function.unoptimized_code();
> ASSERT(!code.IsNull());   // We did EnsureUnoptimizedCode in an earlier phase.
> ResetICs(function, code);

Done.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:1030: func.ZeroEdgeCounters();
On 2016/05/11 19:56:18, rmacnak wrote:
> Should only be needed in the PreserveUnoptimizedCode case.

Acknowledged.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:1059: // the sentinel values so that we have no
stale type feedback.
On 2016/05/11 19:56:18, rmacnak wrote:
> more importantly, no stale method lookups.

Acknowledged.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:1091: HeapIterationScope heap_iteration_scope;
On 2016/05/11 19:56:18, rmacnak wrote:
> ResetUnoptimizedICsOnStack isn't a heap walk. Move the scopes into
> MarkAllFunctionsForRecompilation.

Done.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
File runtime/vm/isolate_reload_test.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/isola...
runtime/vm/isolate_reload_test.cc:1767: #endif  // !PRODUCT
On 2016/05/11 19:56:18, rmacnak wrote:
> Missing cases with schema changes (add/remove fields or add/remove type
> arguments), if only to test they are gracefully rejected for now instead of
> producing some kind of crash.

Added a test cases for field addition and deletion. Added a TODO for generics
related tests.

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/objec...
File runtime/vm/object_reload.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/20001/runtime/vm/objec...
runtime/vm/object_reload.cc:511: void ICData::Reset(bool is_static_call) const {
On 2016/05/11 19:56:18, rmacnak wrote:
> ICData should have an 'is_static_call' bit.

Added TODO

[Cutch, 2016-05-12 16:08:50]

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

https://codereview.chromium.org/1965823002/diff/20001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1734: RawObject* parameterized_class_id_;
On 2016/05/11 19:56:18, rmacnak wrote:
> Why not RawSmi* ?

Doner.

[rmacnak, 2016-05-13 17:59:35]

lgtm w/c, but I must recuse myself on #become:

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/isola...
File runtime/vm/isolate.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/isola...
runtime/vm/isolate.cc:816: background_compiler_disabled_count_(0),
depth

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/isola...
runtime/vm/isolate.cc:2323: OS::Print("Reloading has finished! (%.2f ms)\n",
reload_millis);
if FLAG_trace_reload

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/isola...
File runtime/vm/isolate_reload.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:301: BackgroundCompiler::Disable();
This should happen before DeoptimizeDependentCode.

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/isola...
runtime/vm/isolate_reload.cc:1072: NoSafepointScope no_safepoint;
Push scopes into MarkAllFunctionsForRecompilation

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/objec...
File runtime/vm/object.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/objec...
runtime/vm/object.cc:9282: ? 0 : OS::GetCurrentTimeMillis());
monotonic time?

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/objec...
File runtime/vm/object_reload.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/objec...
runtime/vm/object_reload.cc:400: #if defined(DEBUG)
redundant with the assert

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/objec...
runtime/vm/object_reload.cc:443: }
same number of type arguments too

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/raw_o...
File runtime/vm/raw_object.h (right):

https://chromiumcodereview.appspot.com/1965823002/diff/60001/runtime/vm/raw_o...
runtime/vm/raw_object.h:1043: int64_t load_timestamp_;
initialize in Script::ReadFrom

[Ivan Posva, 2016-05-16 20:58:59]

Here are the comments for Become.

-Ivan

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc
File runtime/vm/become.cc (right):

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
runtime/vm/become.cc:23: static bool IsForwardingObject(RawObject* object) {
Please add a general comment explaining that free list elements are being used
as a marker for forwarding objects. This is safe because we cannot reach free
list elements from live objects.

Ideally we would use a different cid for these as well.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
runtime/vm/become.cc:54: ASSERT(size_before == size_after);
Redundant assert is redundant.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
runtime/vm/become.cc:151: void Become::ElementsForwardIdentity(const Array&
before, const Array& after) {
I am wondering whether this is the right API for this operation. Wouldn't you
want to start a Become operation by allocating a C++ Become object. Then
register the before->after relationships with the Become object, allowing to do
the error checking below when the objects are registered. This way you do not
rely on an external entity to keep the arrays in sync, or you can build up a
hashtable as you go. When you are ready, you just call the Become::Apply method
on the Become object allocated earlier.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
runtime/vm/become.cc:194: if (after_obj->IsFreeListElement()) {
You should be able to drop this. If you ever get an object that is free list
element anything and everything will break.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h
File runtime/vm/become.h (right):

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h#new...
runtime/vm/become.h:18: // in 'after'. Every element in 'before' is guaranteed
to be dead after this
"is guaranteed to be dead" -> "is guaranteed to be not reachable"

And drop the next GC comment as it is not entirely true unless you clear the
before array before triggering the next GC. The "unreachable" comment has the
same problem though.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h#new...
runtime/vm/become.h:28: // static void ElementsExchangeIdentity(const Array&
before,
Drop this as it is not needed for reload.

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

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:669: 
Extra empty line.

[Ivan Posva, 2016-05-17 00:01:04]

On 2016/05/16 20:58:59, Ivan Posva wrote:
> Here are the comments for Become.
> 
> -Ivan
> 
> https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc
> File runtime/vm/become.cc (right):
> 
>
https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
> runtime/vm/become.cc:23: static bool IsForwardingObject(RawObject* object) {
> Please add a general comment explaining that free list elements are being used
> as a marker for forwarding objects. This is safe because we cannot reach free
> list elements from live objects.
> 
> Ideally we would use a different cid for these as well.
> 
>
https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
> runtime/vm/become.cc:54: ASSERT(size_before == size_after);
> Redundant assert is redundant.
> 
>
https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
> runtime/vm/become.cc:151: void Become::ElementsForwardIdentity(const Array&
> before, const Array& after) {
> I am wondering whether this is the right API for this operation. Wouldn't you
> want to start a Become operation by allocating a C++ Become object. Then
> register the before->after relationships with the Become object, allowing to
do
> the error checking below when the objects are registered. This way you do not
> rely on an external entity to keep the arrays in sync, or you can build up a
> hashtable as you go. When you are ready, you just call the Become::Apply
method
> on the Become object allocated earlier.
> 
>
https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
> runtime/vm/become.cc:194: if (after_obj->IsFreeListElement()) {
> You should be able to drop this. If you ever get an object that is free list
> element anything and everything will break.
> 
> https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h
> File runtime/vm/become.h (right):
> 
>
https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h#new...
> runtime/vm/become.h:18: // in 'after'. Every element in 'before' is guaranteed
> to be dead after this
> "is guaranteed to be dead" -> "is guaranteed to be not reachable"
> 
> And drop the next GC comment as it is not entirely true unless you clear the
> before array before triggering the next GC. The "unreachable" comment has the
> same problem though.
> 
>
https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h#new...
> runtime/vm/become.h:28: // static void ElementsExchangeIdentity(const Array&
> before,
> Drop this as it is not needed for reload.
> 
> https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate.h
> File runtime/vm/isolate.h (right):
> 
>
https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate.h#ne...
> runtime/vm/isolate.h:669: 
> Extra empty line.

In case it was not entirely clear: This was the "missing" review of the Become
part.

If you would rather restructure the Become class as part of a different CL that
is entirely fine by me.

-Ivan

[Cutch, 2016-05-17 18:03:52]

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc
File runtime/vm/become.cc (right):

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
runtime/vm/become.cc:23: static bool IsForwardingObject(RawObject* object) {
On 2016/05/16 20:58:59, Ivan Posva wrote:
> Please add a general comment explaining that free list elements are being used
> as a marker for forwarding objects. This is safe because we cannot reach free
> list elements from live objects.
> 
> Ideally we would use a different cid for these as well.

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
runtime/vm/become.cc:54: ASSERT(size_before == size_after);
On 2016/05/16 20:58:59, Ivan Posva wrote:
> Redundant assert is redundant.

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
runtime/vm/become.cc:151: void Become::ElementsForwardIdentity(const Array&
before, const Array& after) {
On 2016/05/16 20:58:59, Ivan Posva wrote:
> I am wondering whether this is the right API for this operation. Wouldn't you
> want to start a Become operation by allocating a C++ Become object. Then
> register the before->after relationships with the Become object, allowing to
do
> the error checking below when the objects are registered. This way you do not
> rely on an external entity to keep the arrays in sync, or you can build up a
> hashtable as you go. When you are ready, you just call the Become::Apply
method
> on the Become object allocated earlier.

Agreed and added a TODO.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.cc#ne...
runtime/vm/become.cc:194: if (after_obj->IsFreeListElement()) {
On 2016/05/16 20:58:59, Ivan Posva wrote:
> You should be able to drop this. If you ever get an object that is free list
> element anything and everything will break.

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h
File runtime/vm/become.h (right):

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h#new...
runtime/vm/become.h:18: // in 'after'. Every element in 'before' is guaranteed
to be dead after this
On 2016/05/16 20:58:59, Ivan Posva wrote:
> "is guaranteed to be dead" -> "is guaranteed to be not reachable"
> 
> And drop the next GC comment as it is not entirely true unless you clear the
> before array before triggering the next GC. The "unreachable" comment has the
> same problem though.

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/become.h#new...
runtime/vm/become.h:28: // static void ElementsExchangeIdentity(const Array&
before,
On 2016/05/16 20:58:59, Ivan Posva wrote:
> Drop this as it is not needed for reload.

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate.cc
File runtime/vm/isolate.cc (right):

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:816: background_compiler_disabled_count_(0),
On 2016/05/13 17:59:34, rmacnak wrote:
> depth

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2323: OS::Print("Reloading has finished! (%.2f ms)\n",
reload_millis);
On 2016/05/13 17:59:34, rmacnak wrote:
> if FLAG_trace_reload

Done.

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

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:669: 
On 2016/05/16 20:58:59, Ivan Posva wrote:
> Extra empty line.

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate_relo...
File runtime/vm/isolate_reload.cc (right):

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:1072: NoSafepointScope no_safepoint;
On 2016/05/13 17:59:34, rmacnak wrote:
> Push scopes into MarkAllFunctionsForRecompilation

Done.

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

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9282: ? 0 : OS::GetCurrentTimeMillis());
On 2016/05/13 17:59:34, rmacnak wrote:
> monotonic time?

Sticking with non-monotonic. We report this time to the user in Observatory.
Using the monotonic clock would help us avoid the unlikely case of a clock roll
back that conflicts with a previous load time but would stop us from being able
to display the load time to the developer.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/object_reloa...
File runtime/vm/object_reload.cc (right):

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/object_reloa...
runtime/vm/object_reload.cc:400: #if defined(DEBUG)
On 2016/05/13 17:59:34, rmacnak wrote:
> redundant with the assert

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/object_reloa...
runtime/vm/object_reload.cc:443: }
On 2016/05/13 17:59:34, rmacnak wrote:
> same number of type arguments too

Done.

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/1965823002/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:1043: int64_t load_timestamp_;
On 2016/05/13 17:59:34, rmacnak wrote:
> initialize in Script::ReadFrom

Done.

[Cutch, 2016-05-17 19:19:10]

Description was changed from

==========
Initial isolate reload support

This is a cut of the work that Todd and I collaborated on in the reload branch.

In this CL, we've dropped the loader port hacks, in other words, on stack
reloading in the standalone embedder does not work yet. 

- [x] Support for hot reloading of isolate source code
- [x] Unit test harness and many tests
- [x] Service protocol and Observatory support
- [x] Product build does not include support for hot reloading.
==========

to

==========
Initial isolate reload support

This is a cut of the work that Todd and I collaborated on in the reload branch.

In this CL, we've dropped the loader port hacks, in other words, on stack
reloading in the standalone embedder does not work yet.

- [x] Support for hot reloading of isolate source code
- [x] Unit test harness and many tests
- [x] Service protocol and Observatory support
- [x] Product build does not include support for hot reloading.

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/48c8ffa7f3d778fb6ee39c558f7ffae9a134e7ad
==========

[Cutch, 2016-05-17 19:19:12]

Committed patchset #6 (id:100001) manually as
48c8ffa7f3d778fb6ee39c558f7ffae9a134e7ad (presubmit successful).

[Florian Schneider, 2016-05-18 09:36:55]

fschneider@google.com changed reviewers:
+ fschneider@google.com

[Florian Schneider, 2016-05-18 09:36:57]

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

https://codereview.chromium.org/1965823002/diff/100001/runtime/vm/object.cc#n...
runtime/vm/object.cc:12791: }
This approach does not work with background compilation:

ic_data() may be updated by the mutator thread, while iterating in the loop
above.

If that happens -- i.e. ic_data was updated, we won't find a sentinel at the end
of the ic_data() array and crash here.

[Vyacheslav Egorov (Google), 2016-05-18 10:28:42]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2016-05-18 10:28:43]

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

https://codereview.chromium.org/1965823002/diff/100001/runtime/vm/object.h#ne...
runtime/vm/object.h:1951: void ValidateSentinelLocations() const;
This function is empty.

[siva, 2016-05-27 23:47:59]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2016-05-27 23:48:01]

https://chromiumcodereview.appspot.com/1965823002/diff/100001/runtime/vm/beco...
File runtime/vm/become.cc (right):

https://chromiumcodereview.appspot.com/1965823002/diff/100001/runtime/vm/beco...
runtime/vm/become.cc:25: // forwarding objects would have their own class id.
See TODO below.
We should change this to have a becomes forwarding object instead of overloading
Free list elements like this.