vm/observatory: Clean up script access

vm/observatory: Clean up script access

Addresses issues from https://codereview.chromium.org/346003003/

BUG=
R=hausner@google.com, johnmccutchan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=38264

[Michael Lippautz (Google), 2014-07-15 17:52:55]

Assumption: Script objects may not be shared across libraries (which is the case
as of today).

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

https://codereview.chromium.org/392933003/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:8323: return Library::null();
This is not UNREACHABLE(), as we can still use scripts on their own, e.g. see
runtime/vm/report_test.cc

https://codereview.chromium.org/392933003/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:8896: RawArray* Library::LoadedScripts(bool cached) const {
This whole function could be rewritten to use ClassDictionaryIterator() and
iterate over (anon) classes and their functions/fields. Opinions?

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

https://codereview.chromium.org/392933003/diff/20001/runtime/vm/object.h#newc...
runtime/vm/object.h:2680: RawArray* LoadedScripts(bool cached = true) const;
Make parameter explicit?

LoadedScripts() uses a cache which is not ideal in the case of lazy loading.

[hausner, 2014-07-15 20:34:03]

My main concern is that this may result in a memory leak. See comment below.

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

https://codereview.chromium.org/392933003/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:8899: if (!cached || loaded_scripts() == Array::null()) {
I'm afraid this may have unintended side effects. Since LoadedScripts was
initially written, the VM now produces lots of tiny scripts as a side effect of
evaluating expressions. You probably find all these scripts and store them in
loaded_scripts_ each time LoadedScripts(false) is called. This means we hang on
to the script objects and they no longer get GCed. Also, when the debugger asks
for the list of loaded scripts, we send all these throw-away scripts that were
produced.

[Michael Lippautz (Google), 2014-07-15 21:20:56]

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

https://codereview.chromium.org/392933003/diff/20001/runtime/vm/object.cc#new...
runtime/vm/object.cc:8899: if (!cached || loaded_scripts() == Array::null()) {
On 2014/07/15 20:34:03, hausner wrote:
> I'm afraid this may have unintended side effects. Since LoadedScripts was
> initially written, the VM now produces lots of tiny scripts as a side effect
of
> evaluating expressions. You probably find all these scripts and store them in
> loaded_scripts_ each time LoadedScripts(false) is called. This means we hang
on
> to the script objects and they no longer get GCed. Also, when the debugger
asks
> for the list of loaded scripts, we send all these throw-away scripts that were
> produced.

Thanks for the (offline) clarifications. The cache is not needed for this CL.
While libraries may be deferred, scripts imported in a library are loaded all at
once.

[hausner, 2014-07-15 21:38:18]

LGTM w/Q

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

https://codereview.chromium.org/392933003/diff/40001/runtime/vm/object.cc#new...
runtime/vm/object.cc:8931: for (intptr_t i = 0; i < anon_classes.Length(); i++)
{
What scripts do you find in anon_classes that are not already found through the
library dictionary above? Can you please add a comment to explain this?

[Cutch, 2014-07-15 22:06:55]

https://codereview.chromium.org/392933003/diff/40001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/service/object.dart (right):

https://codereview.chromium.org/392933003/diff/40001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/service/object.dart:1371:
_parseTokenPosTable(map['tokenPosTable']);
There should be a "if (mapIsRef) { return; }" statement somewhere in here.

https://codereview.chromium.org/392933003/diff/40001/runtime/vm/report_test.cc
File runtime/vm/report_test.cc (right):

https://codereview.chromium.org/392933003/diff/40001/runtime/vm/report_test.c...
runtime/vm/report_test.cc:33: "\"libraries\\/-1\\/scripts\\/Plug\","
Why is this -1?

[Michael Lippautz (Google), 2014-07-15 22:40:42]

https://codereview.chromium.org/392933003/diff/40001/runtime/bin/vmservice/cl...
File runtime/bin/vmservice/client/lib/src/service/object.dart (right):

https://codereview.chromium.org/392933003/diff/40001/runtime/bin/vmservice/cl...
runtime/bin/vmservice/client/lib/src/service/object.dart:1371:
_parseTokenPosTable(map['tokenPosTable']);
On 2014/07/15 22:06:54, Cutch wrote:
> There should be a "if (mapIsRef) { return; }" statement somewhere in here.

Done.

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

https://codereview.chromium.org/392933003/diff/40001/runtime/vm/object.cc#new...
runtime/vm/object.cc:8931: for (intptr_t i = 0; i < anon_classes.Length(); i++)
{
On 2014/07/15 21:38:18, hausner wrote:
> What scripts do you find in anon_classes that are not already found through
the
> library dictionary above? Can you please add a comment to explain this?

Added comment to code:
// Special case: Scripts that only contain external top-level functions are
// not include above, but can be referenced through a library's anonymous
// classes. Example: dart-core:identical.dart.

https://codereview.chromium.org/392933003/diff/40001/runtime/vm/report_test.cc
File runtime/vm/report_test.cc (right):

https://codereview.chromium.org/392933003/diff/40001/runtime/vm/report_test.c...
runtime/vm/report_test.cc:33: "\"libraries\\/-1\\/scripts\\/Plug\","
On 2014/07/15 22:06:55, Cutch wrote:
> Why is this -1?

script.Tokenize() does not process the script into the vm infrastructure, i.e.,
it does not assign it a library etc.

In this case we use -1 as library id in the JSON representation.

[Cutch, 2014-07-15 22:41:52]

lgtm

[Michael Lippautz (Google), 2014-07-15 22:55:30]

Committed patchset #4 manually as r38264 (presubmit successful).