Perform a full GC by default when requesting a heap snapshot.

Perform a full GC by default when requesting a heap snapshot.

Prevents the snapshot from include objects that are only weakly
reachable.

BUG=http://dartbug.com/25994
R=johnmccutchan@google.com

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

[rmacnak, 2016-03-16 22:59:14]

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

[Cutch, 2016-03-17 15:37:32]

LGTM with a couple comments/suggestions

https://codereview.chromium.org/1811713002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/heap_snapshot.dart (right):

https://codereview.chromium.org/1811713002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/heap_snapshot.dart:412:
app.locationManager.uri.queryParameters['collectGarbage'];
Maybe a helper function like bool getBoolParameter(String name, bool
defaultValue);

app.locationManager.getBoolParameter('collectGarbage', true) ?

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

https://codereview.chromium.org/1811713002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3279:
BoolParameter::Parse(js->LookupParam("collectGarbage"), true);
should the default be true?

[rmacnak, 2016-03-17 18:09:09]

https://codereview.chromium.org/1811713002/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/elements/heap_snapshot.dart (right):

https://codereview.chromium.org/1811713002/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/elements/heap_snapshot.dart:412:
app.locationManager.uri.queryParameters['collectGarbage'];
On 2016/03/17 15:37:32, Cutch wrote:
> Maybe a helper function like bool getBoolParameter(String name, bool
> defaultValue);
> 
> app.locationManager.getBoolParameter('collectGarbage', true) ?

Done.

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

https://codereview.chromium.org/1811713002/diff/20001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3279:
BoolParameter::Parse(js->LookupParam("collectGarbage"), true);
On 2016/03/17 15:37:32, Cutch wrote:
> should the default be true?

I think so.

[rmacnak, 2016-03-17 18:11:41]

Description was changed from

==========
Perform a full GC by default when requesting a heap snapshot.

Prevents the snapshot from include objects that are only weakly
reachable.

BUG=http://dartbug.com/25994
==========

to

==========
Perform a full GC by default when requesting a heap snapshot.

Prevents the snapshot from include objects that are only weakly
reachable.

BUG=http://dartbug.com/25994
R=johnmccutchan@google.com

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

[rmacnak, 2016-03-17 18:11:43]

Committed patchset #3 (id:40001) manually as
6eaa45058c8879ef9e32d9f356b3ef7e400e3a43 (presubmit successful).

[Ivan Posva, 2016-03-18 23:38:58]

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

[Ivan Posva, 2016-03-18 23:38:58]

DBC

-Ivan

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

https://codereview.chromium.org/1811713002/diff/40001/runtime/vm/object_graph...
runtime/vm/object_graph.cc:519: isolate()->heap()->new_space()->Evacuate();
You could also run a scavenge followed by the call to CollectAllGarbage above.
This should have the same effect as evacuating after the full collection with
the additional benefit of collecting new-old-new cycles.