Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2574643003:
Added ability to request zone memory information for all isolates through the VM service and added … (Closed)
|
Created:
4 years ago by bkonyi Modified:
4 years ago CC:
reviews_dartlang.org, turnidge, rmacnak, vm-dev_dartlang.org Target Ref:
refs/heads/master Visibility:
Public. |
DescriptionAdded ability to request zone memory information for all isolates through the VM service and added tests.
BUG=
R=johnmccutchan@google.com, zra@google.com
Committed: https://github.com/dart-lang/sdk/commit/9622f1a2dfe6aeafb6928bc9dabead5f0ecbdef5
Patch Set 1 #
Total comments: 10
Patch Set 2 : Added dart test to check that Isolates contain thread + zone fields required for zone memory report… #
Total comments: 6
Patch Set 3 : Created dart objects for thread and zone and added threads field to isolate. #
Total comments: 20
Patch Set 4 : Converted to using strong types, removed unused code and whitespace. #
Total comments: 10
Patch Set 5 : Addressed bugs found in last patch, converted additional variables to use strong types, updated com… #
Messages
Total messages: 20 (2 generated)
bkonyi@google.com changed reviewers: + johnmccutchan@google.com, zra@google.com
https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc File runtime/vm/service.cc (right): https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc#newco... runtime/vm/service.cc:2974: jsobj.AddProperty("type", "_ZoneMemoryInfo"); There's probably a better name for this type, but I couldn't think of one off the top of my head. If you have suggestions, let me know. https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc#newco... runtime/vm/service.cc:4107: { "_getZoneMemoryInfo", GetZoneMemoryInfo, I'm not sure if this is the best name since what's really being returned is an object containing an array of Isolate objects, which just happens to contain all the thread + zone information. Would it make more sense to call it something to do with getting all isolate information?
https://codereview.chromium.org/2574643003/diff/1/runtime/observatory/tests/s... File runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart (right): https://codereview.chromium.org/2574643003/diff/1/runtime/observatory/tests/s... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:1: // Copyright (c) 2015, the Dart project authors. Please see the AUTHORS file 2016 https://codereview.chromium.org/2574643003/diff/1/runtime/observatory/tests/s... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:7: import 'package:unittest/unittest.dart'; Should we be using the test rather than unittest package? I thought unittest was deprecated. Not sure about his though. https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc File runtime/vm/service.cc (right): https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc#newco... runtime/vm/service.cc:2951: explicit ServiceIsolateVisitor(JSONArray* jsarr, bool ref = true) Since there are still only a small number of uses of this, I think we should avoid using optional arguments. https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc#newco... runtime/vm/service.cc:2974: jsobj.AddProperty("type", "_ZoneMemoryInfo"); On 2016/12/13 22:37:48, bkonyi wrote: > There's probably a better name for this type, but I couldn't think of one off > the top of my head. If you have suggestions, let me know. _AllIsolatesInfo? Not sure if this matches with the service's naming conventions. Have to ask John. https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc#newco... runtime/vm/service.cc:4107: { "_getZoneMemoryInfo", GetZoneMemoryInfo, On 2016/12/13 22:37:48, bkonyi wrote: > I'm not sure if this is the best name since what's really being returned is an > object containing an array of Isolate objects, which just happens to contain all > the thread + zone information. Would it make more sense to call it something to > do with getting all isolate information? If there isn't already something like that, then that would make sense to me, but wait for John to comment.
https://codereview.chromium.org/2574643003/diff/1/runtime/observatory/tests/s... File runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart (right): https://codereview.chromium.org/2574643003/diff/1/runtime/observatory/tests/s... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:7: import 'package:unittest/unittest.dart'; On 2016/12/13 23:37:02, zra wrote: > Should we be using the test rather than unittest package? I thought unittest was > deprecated. Not sure about his though. I'm not sure myself. I had copied this test originally from get_vm_rpc_test.dart, so that's what was there. https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc File runtime/vm/service.cc (right): https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc#newco... runtime/vm/service.cc:2951: explicit ServiceIsolateVisitor(JSONArray* jsarr, bool ref = true) On 2016/12/13 23:37:02, zra wrote: > Since there are still only a small number of uses of this, I think we should > avoid using optional arguments. Done.
This isn't the right approach. Let's discuss offline. https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc File runtime/vm/service.cc (right): https://codereview.chromium.org/2574643003/diff/1/runtime/vm/service.cc#newco... runtime/vm/service.cc:2972: static bool GetZoneMemoryInfo(Thread* thread, JSONStream* js) { This is the same as getVM but with the full Isolate information for each isolate instead of just a reference. A client can easily do this themselves.
As discussed offline, I've gone ahead and removed the new RPC call I created and just created a test to check the Isolate objects contain the correct thread + zone fields.
lgtm
https://codereview.chromium.org/2574643003/diff/20001/runtime/observatory/tes... File runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart (right): https://codereview.chromium.org/2574643003/diff/20001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:13: var isInt = new isInstanceOf<int>(); please use strong mode types here and elsewhere. https://codereview.chromium.org/2574643003/diff/20001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:17: var params = { Our object model doesn't make a distinction between an Isolate and an IsolateRef. simply writing: for (Isolate isolate in vm.isolates) { await isolate.reload(); <-- ensure you have the full isolate and not a ref. // expect test code goes here. } https://codereview.chromium.org/2574643003/diff/20001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:22: expect(isolate.containsKey('threads'), isTrue); Please just extend the Isolate model to include thread information. You extract information from the JSON response and put it into the Isolate model here: https://github.com/dart-lang/sdk/blob/master/runtime/observatory/lib/src/serv...
I've gone ahead and addressed John's comments from the last patch, but I'm not expecting this to actually be the final patch for this CL. https://codereview.chromium.org/2574643003/diff/20001/runtime/observatory/tes... File runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart (right): https://codereview.chromium.org/2574643003/diff/20001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:13: var isInt = new isInstanceOf<int>(); On 2016/12/14 23:10:48, Cutch wrote: > please use strong mode types here and elsewhere. Done. https://codereview.chromium.org/2574643003/diff/20001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:17: var params = { On 2016/12/14 23:10:48, Cutch wrote: > Our object model doesn't make a distinction between an Isolate and an > IsolateRef. > > simply writing: > > for (Isolate isolate in vm.isolates) { > await isolate.reload(); <-- ensure you have the full isolate and not a ref. > // expect test code goes here. > } Done. https://codereview.chromium.org/2574643003/diff/20001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:22: expect(isolate.containsKey('threads'), isTrue); On 2016/12/14 23:10:48, Cutch wrote: > Please just extend the Isolate model to include thread information. > > You extract information from the JSON response and put it into the Isolate model > here: > > https://github.com/dart-lang/sdk/blob/master/runtime/observatory/lib/src/serv... Done.
https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... File runtime/observatory/lib/src/models/objects/zone.dart (right): https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/models/objects/zone.dart:10: num get capacity; int here and below https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... File runtime/observatory/lib/src/service/object.dart (right): https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:7: we usually don't add new whitespace https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:1509: List<Thread> _threads = new List<Thread>(); final List<Thread> _threads ... https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:1635: List threadsList = map['threads']; follow the pattern from above: threads.clear(); if (map['threadsList'] != null) { threads.addAll(map['threadsList']); } https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3075: List<Zone> _zones = new List<Zone>(); final List<Zone> ... https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3083: switch(kindString) { is everything guaranteed to be sent even when this is a ref? https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3106: num capacity = zone['capacity']; int here and elsewhere https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/tes... File runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart (right): https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:18: var params = { why is this here? It's not being used. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:23: var threads = isolate.threads; use strong mode types here and elsewhere https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:25: for (var thread in threads) { use strong mode types here and elsewhere.
https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... File runtime/observatory/lib/src/models/objects/zone.dart (right): https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/models/objects/zone.dart:10: num get capacity; On 2016/12/16 22:55:43, Cutch wrote: > int here and below Done. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... File runtime/observatory/lib/src/service/object.dart (right): https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:7: On 2016/12/16 22:55:44, Cutch wrote: > we usually don't add new whitespace This was accidental. Removed. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3083: switch(kindString) { On 2016/12/16 22:55:44, Cutch wrote: > is everything guaranteed to be sent even when this is a ref? Currently Thread doesn't have a ref version. Should that also be an option? Threads are only populated in an Isolate object when it isn't a reference. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3106: num capacity = zone['capacity']; On 2016/12/16 22:55:43, Cutch wrote: > int here and elsewhere Right, done. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/tes... File runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart (right): https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:18: var params = { On 2016/12/16 22:55:44, Cutch wrote: > why is this here? It's not being used. Forgot to remove this... whoops. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:23: var threads = isolate.threads; On 2016/12/16 22:55:44, Cutch wrote: > use strong mode types here and elsewhere Acknowledged. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/tes... runtime/observatory/tests/service/get_zone_memory_info_rpc_test.dart:25: for (var thread in threads) { On 2016/12/16 22:55:44, Cutch wrote: > use strong mode types here and elsewhere. Acknowledged.
https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... File runtime/observatory/lib/src/models/objects/thread.dart (right): https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/models/objects/thread.dart:8: kUnknownTask, https://www.dartlang.org/guides/language/effective-dart/style#prefer-using-lo... unknownTask mutatorTask ... https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... File runtime/observatory/lib/src/models/objects/zone.dart (right): https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/models/objects/zone.dart:9: /// memory that is not actually being used. here and elsewhere: describe the units of memory. Are these words, bytes, megs? https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... File runtime/observatory/lib/src/service/object.dart (right): https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:1634: List threadsList = map['threads']; please address all comments. From my previous review: map['threads']; follow the pattern from above: threads.clear(); if (map['threadsList'] != null) { threads.addAll(map['threadsList']); } https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3080: List zoneList = map['zones']; use strong mode types here and elsewhere https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3104: zoneList.forEach((zone) { This will repeatedly add to the zone list. Follow the pattern used elsewhere: zones.clear(); ...
I've addressed the issues raised by John in the last two patches including not clearing thread/isolate lists before loading updated information, some variables still not using explicit types, enums not conforming to the Dart style guide, and not being explicit in the unit of memory used within the Zone class. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... File runtime/observatory/lib/src/service/object.dart (right): https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:1509: List<Thread> _threads = new List<Thread>(); On 2016/12/16 22:55:43, Cutch wrote: > final List<Thread> _threads ... Done. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:1635: List threadsList = map['threads']; On 2016/12/16 22:55:43, Cutch wrote: > follow the pattern from above: > > threads.clear(); > if (map['threadsList'] != null) { > threads.addAll(map['threadsList']); > } Done. https://codereview.chromium.org/2574643003/diff/40001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3075: List<Zone> _zones = new List<Zone>(); On 2016/12/16 22:55:44, Cutch wrote: > final List<Zone> ... Done. https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... File runtime/observatory/lib/src/models/objects/thread.dart (right): https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/models/objects/thread.dart:8: kUnknownTask, On 2016/12/19 14:46:09, Cutch wrote: > https://www.dartlang.org/guides/language/effective-dart/style#prefer-using-lo... > > unknownTask > mutatorTask > ... Done. https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... File runtime/observatory/lib/src/models/objects/zone.dart (right): https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/models/objects/zone.dart:9: /// memory that is not actually being used. On 2016/12/19 14:46:09, Cutch wrote: > here and elsewhere: describe the units of memory. Are these words, bytes, megs? Done. https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... File runtime/observatory/lib/src/service/object.dart (right): https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:1634: List threadsList = map['threads']; On 2016/12/19 14:46:09, Cutch wrote: > please address all comments. From my previous review: > > map['threads']; > follow the pattern from above: > > threads.clear(); > if (map['threadsList'] != null) { > threads.addAll(map['threadsList']); > } Sorry, I had thought I had done this before. Done. https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3080: List zoneList = map['zones']; On 2016/12/19 14:46:09, Cutch wrote: > use strong mode types here and elsewhere Done. https://codereview.chromium.org/2574643003/diff/60001/runtime/observatory/lib... runtime/observatory/lib/src/service/object.dart:3104: zoneList.forEach((zone) { On 2016/12/19 14:46:09, Cutch wrote: > This will repeatedly add to the zone list. Follow the pattern used elsewhere: > > zones.clear(); > ... Same as above. Done.
On 2016/12/19 16:40:20, bkonyi wrote: > I've addressed the issues raised by John in the last two patches including not > clearing thread/isolate lists before loading updated information, some variables > still not using explicit types, enums not conforming to the Dart style guide, > and not being explicit in the unit of memory used within the Zone class. > Publishing is a request for your reviewers to look at the code again. Try and avoid publishing comments until you've completely addressed all of the comments.
On 2016/12/19 17:29:20, Cutch wrote: > On 2016/12/19 16:40:20, bkonyi wrote: > > I've addressed the issues raised by John in the last two patches including not > > clearing thread/isolate lists before loading updated information, some > variables > > still not using explicit types, enums not conforming to the Dart style guide, > > and not being explicit in the unit of memory used within the Zone class. > > > > Publishing is a request for your reviewers to look at the code again. Try and > avoid publishing comments until you've completely addressed all of the comments. Did I not address the comments in the most recent patch? I've only been publishing after submitting changes that need to be reviewed, unless I'm doing something weird.
On 2016/12/19 17:35:28, bkonyi wrote: > On 2016/12/19 17:29:20, Cutch wrote: > > On 2016/12/19 16:40:20, bkonyi wrote: > > > I've addressed the issues raised by John in the last two patches including > not > > > clearing thread/isolate lists before loading updated information, some > > variables > > > still not using explicit types, enums not conforming to the Dart style > guide, > > > and not being explicit in the unit of memory used within the Zone class. > > > > > > > Publishing is a request for your reviewers to look at the code again. Try and > > avoid publishing comments until you've completely addressed all of the > comments. > > Did I not address the comments in the most recent patch? I've only been > publishing after submitting changes that need to be reviewed, unless I'm doing > something weird. No, it was my bad this time.
LGTM!
Description was changed from ========== Added ability to request zone memory information for all isolates through the VM service and added tests. BUG= ========== to ========== Added ability to request zone memory information for all isolates through the VM service and added tests. BUG= R=johnmccutchan@google.com, zra@google.com Committed: https://github.com/dart-lang/sdk/commit/9622f1a2dfe6aeafb6928bc9dabead5f0ecbdef5 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) manually as 9622f1a2dfe6aeafb6928bc9dabead5f0ecbdef5 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
