[kernel] Free memory for kernel data.

[kernel] Free memory for kernel data.

This should cleanup user script kernel data and service isolate kernel data,
but leave the platform kernel data alone as it is used several times,
and it is unclear to me when it is safe to free it.

BUG=
R=kmillikin@google.com

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

[jensj, 2017-08-09 13:08:22]

jensj@google.com changed reviewers:
+ kmillikin@google.com

[Kevin Millikin (Google), 2017-08-14 06:20:50]

LGTM with comments addressed.

https://codereview.chromium.org/3000623002/diff/20001/runtime/bin/dfe.cc
File runtime/bin/dfe.cc (right):

https://codereview.chromium.org/3000623002/diff/20001/runtime/bin/dfe.cc#newc...
runtime/bin/dfe.cc:45: void DFE::unset_kernel_vmservice_io() {
"unset" isn't really a word.  Usually "clear" is the opposite of "set", though
here I'd probably use something like ReleaseVMServiceIO to make it seem more
like it's giving up ownership.

https://codereview.chromium.org/3000623002/diff/20001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/3000623002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5063: program = NULL;
program = NULL and kernel = NULL don't seem necessary.

If you were worried that this function might grow a lot more code that used
program after it was deleted, you could put it in its own scope.  (But I
wouldn't worry too much about that.)

https://codereview.chromium.org/3000623002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5249: program = NULL;
Likewise, here I don't think setting them to NULL is really needed.

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

https://codereview.chromium.org/3000623002/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:120: if (kernel_data_ != NULL) {
free checks for null.  It's clunky to check here, so don't.

[Kevin Millikin (Google), 2017-08-14 06:20:52]

LGTM with comments addressed.

[jensj, 2017-08-14 08:38:12]

https://codereview.chromium.org/3000623002/diff/20001/runtime/bin/dfe.cc
File runtime/bin/dfe.cc (right):

https://codereview.chromium.org/3000623002/diff/20001/runtime/bin/dfe.cc#newc...
runtime/bin/dfe.cc:45: void DFE::unset_kernel_vmservice_io() {
On 2017/08/14 06:20:50, Kevin Millikin (Google) wrote:
> "unset" isn't really a word.  Usually "clear" is the opposite of "set", though
> here I'd probably use something like ReleaseVMServiceIO to make it seem more
> like it's giving up ownership.

Renaming to 'clear_kernel_vmservice_io' as I find 'Release' seems like it
releases resources (e.g. frees the memory), which it doesn't.

https://codereview.chromium.org/3000623002/diff/20001/runtime/vm/dart_api_imp...
File runtime/vm/dart_api_impl.cc (right):

https://codereview.chromium.org/3000623002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5063: program = NULL;
On 2017/08/14 06:20:50, Kevin Millikin (Google) wrote:
> program = NULL and kernel = NULL don't seem necessary.
> 
> If you were worried that this function might grow a lot more code that used
> program after it was deleted, you could put it in its own scope.  (But I
> wouldn't worry too much about that.)

Done.

https://codereview.chromium.org/3000623002/diff/20001/runtime/vm/dart_api_imp...
runtime/vm/dart_api_impl.cc:5249: program = NULL;
On 2017/08/14 06:20:50, Kevin Millikin (Google) wrote:
> Likewise, here I don't think setting them to NULL is really needed.

Done.

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

https://codereview.chromium.org/3000623002/diff/20001/runtime/vm/kernel.h#new...
runtime/vm/kernel.h:120: if (kernel_data_ != NULL) {
On 2017/08/14 06:20:50, Kevin Millikin (Google) wrote:
> free checks for null.  It's clunky to check here, so don't.

Done.

[jensj, 2017-08-14 08:38:21]

Description was changed from

==========
[kernel] Free memory for kernel data.

This should cleanup user script kernel data and service isolate kernel data,
but leave the platform kernel data alone as it is used several times,
and it is unclear to me when it is safe to free it.

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

to

==========
[kernel] Free memory for kernel data.

This should cleanup user script kernel data and service isolate kernel data,
but leave the platform kernel data alone as it is used several times,
and it is unclear to me when it is safe to free it.

BUG=
R=kmillikin@google.com

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

[jensj, 2017-08-14 08:38:23]

Committed patchset #2 (id:20001) manually as
c7faf2dc6b64b65e04d677fa3ad68c1e90e19bff (presubmit successful).