Implemented schema change for the isolate reload operation.

Implemented schema change for the isolate reload operation.
I'll add more tests to isolate_reload_test.cc while waiting for the first round of comments.

BUG=
R=johnmccutchan@google.com

Committed: https://github.com/dart-lang/sdk/commit/293c6116e94addd6e8897fc64fd84a9638b3f536

[bakster, 2016-07-25 21:29:29]

bak@google.com changed reviewers:
+ johnmccutchan@google.com

[bakster, 2016-07-25 21:29:29]

Hi John

Could you give me the first round of comments while I add more test cases?

Thanks,
  Lars

[bakster, 2016-07-25 22:39:27]

bak@google.com changed reviewers:
+ turnidge@google.com

[bakster, 2016-07-25 22:39:27]

Forgot to include Todd.

[Cutch, 2016-07-26 14:42:46]

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

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:129: OS::Print("%s\n", cls.ToCString());
Use a LogBlock scope object and THR_Print so multi-threaded printing works.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:156: OS::Print("Morphing from ");
here too

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:173: OS::Print("[[%s]]\n", message.ToCString());
We should print these to a JSONStream object instead of to the console.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:189: OS::Print("[[%s]]\n", error.ToCString());
We should print these to a JSONStream object instead of to the console.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:949: OS::Print("MorphInstance: \n");
Use TIR_Print (it checks FLAG_trace_reload for you)

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:964: OS::Print("Found %" Pd " object%s subject to
morphing.\n",
TIR_Print

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:1041: if (HasInstanceMorphers()) {
This should be done in the commit path

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
File runtime/vm/isolate_reload_test.cc (right):

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload_test.cc:2072: }
We should add some negative (failures) test cases too.

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

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/object_reloa...
runtime/vm/object_reload.cc:490: context->AddReasonForCancelling(
This way of reporting errors is much nicer than what we had before!

[bakster, 2016-07-26 16:54:51]

PTAL

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

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:129: OS::Print("%s\n", cls.ToCString());
On 2016/07/26 14:42:45, Cutch wrote:
> Use a LogBlock scope object and THR_Print so multi-threaded printing works.

Done.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:156: OS::Print("Morphing from ");
On 2016/07/26 14:42:45, Cutch wrote:
> here too

Done.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:189: OS::Print("[[%s]]\n", error.ToCString());
On 2016/07/26 14:42:45, Cutch wrote:
> We should print these to a JSONStream object instead of to the console.

For now, I removed the print statement since it is leftover code.
Let work on JSON conversion in my next commit.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:949: OS::Print("MorphInstance: \n");
On 2016/07/26 14:42:45, Cutch wrote:
> Use TIR_Print (it checks FLAG_trace_reload for you)

Done.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:964: OS::Print("Found %" Pd " object%s subject to
morphing.\n",
On 2016/07/26 14:42:45, Cutch wrote:
> TIR_Print

Done.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:1041: if (HasInstanceMorphers()) {
On 2016/07/26 14:42:45, Cutch wrote:
> This should be done in the commit path

Done.

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
File runtime/vm/isolate_reload_test.cc (right):

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload_test.cc:2072: }
On 2016/07/26 14:42:46, Cutch wrote:
> We should add some negative (failures) test cases too.

I'll add negative tests testing changing type parameters result in errors.

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

https://codereview.chromium.org/2179983002/diff/20001/runtime/vm/object_reloa...
runtime/vm/object_reload.cc:490: context->AddReasonForCancelling(
On 2016/07/26 14:42:46, Cutch wrote:
> This way of reporting errors is much nicer than what we had before!
Thanks

[Cutch, 2016-07-26 17:10:16]

LGTM after final set of comments are addressed

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

https://codereview.chromium.org/2179983002/diff/40001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:129: LogBlock blocker;
Remove this LogBlock -- the one in Dump should be sufficient.

https://codereview.chromium.org/2179983002/diff/40001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:750: MorphInstances();
Calling MorphInstances resets the saved_class_table. This should probably be
moved further down in this function.

https://codereview.chromium.org/2179983002/diff/40001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:834: 
call MorphInstances here instead

https://codereview.chromium.org/2179983002/diff/40001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:946: private:
blank line between count() and private:

[bakster, 2016-07-26 18:01:47]

Thanks,
  Lars

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

https://codereview.chromium.org/2179983002/diff/40001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:129: LogBlock blocker;
On 2016/07/26 17:10:16, Cutch wrote:
> Remove this LogBlock -- the one in Dump should be sufficient.

Done.

https://codereview.chromium.org/2179983002/diff/40001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:750: MorphInstances();
On 2016/07/26 17:10:16, Cutch wrote:
> Calling MorphInstances resets the saved_class_table. This should probably be
> moved further down in this function.

The placement will be documents.

https://codereview.chromium.org/2179983002/diff/40001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:834: 
On 2016/07/26 17:10:16, Cutch wrote:
> call MorphInstances here instead

Acknowledged.

https://codereview.chromium.org/2179983002/diff/40001/runtime/vm/isolate_relo...
runtime/vm/isolate_reload.cc:946: private:
On 2016/07/26 17:10:16, Cutch wrote:
> blank line between count() and private:

Done.

[bakster, 2016-07-26 18:13:33]

Description was changed from

==========
Implemented schema change for the isolate reload operation.
I'll add more tests to isolate_reload_test.cc while waiting for the first round
of comments.

BUG=
==========

to

==========
Implemented schema change for the isolate reload operation.
I'll add more tests to isolate_reload_test.cc while waiting for the first round
of comments.

BUG=
R=johnmccutchan@google.com

Committed:
https://github.com/dart-lang/sdk/commit/293c6116e94addd6e8897fc64fd84a9638b3f536
==========

[bakster, 2016-07-26 18:13:34]

Committed patchset #4 (id:60001) manually as
293c6116e94addd6e8897fc64fd84a9638b3f536 (presubmit successful).