Major rework of vm service events.

runtime/observatory/lib/src/service/object.dart

Index: runtime/observatory/lib/src/service/object.dart
diff --git a/runtime/observatory/lib/src/service/object.dart b/runtime/observatory/lib/src/service/object.dart
index 925e0e75eba7d5cfaaadc88d4d4a2ee261ccb130..ca053a1456bccc6bc7f730cf10656c32f6bc543c 100644
--- a/runtime/observatory/lib/src/service/object.dart
+++ b/runtime/observatory/lib/src/service/object.dart
@@ -364,16 +364,16 @@ abstract class VM extends ServiceObjectOwner {
       new StreamController.broadcast();
 
   bool _isIsolateLifecycleEvent(String eventType) {
-    return _isIsolateShutdownEvent(eventType) ||
-           _isIsolateCreatedEvent(eventType);
+    return _isIsolateExitEvent(eventType) ||
+           _isIsolateStartEvent(eventType);
   }
 
-  bool _isIsolateShutdownEvent(String eventType) {
-    return (eventType == 'IsolateShutdown');
+  bool _isIsolateExitEvent(String eventType) {
+    return (eventType == 'IsolateExit');
   }
 
-  bool _isIsolateCreatedEvent(String eventType) {
-    return (eventType == 'IsolateCreated');
+  bool _isIsolateStartEvent(String eventType) {
+    return (eventType == 'IsolateStart');
   }
 
   void postServiceEvent(String response, ByteData data) {
@@ -399,17 +399,17 @@ abstract class VM extends ServiceObjectOwner {
     if (_isIsolateLifecycleEvent(eventType)) {
       String isolateId = map['isolate']['id'];
       var event;
-      if (_isIsolateCreatedEvent(eventType)) {
-        _onIsolateCreated(map['isolate']);
+      if (_isIsolateStartEvent(eventType)) {
+        _onIsolateStart(map['isolate']);
         // By constructing the event *after* adding the isolate to the
         // isolate cache, the call to getFromMap will use the cached Isolate.
         event = new ServiceObject._fromMap(this, map);
       } else {
-        assert(_isIsolateShutdownEvent(eventType));
+        assert(_isIsolateExitEvent(eventType));
         // By constructing the event *before* removing the isolate from the
         // isolate cache, the call to getFromMap will use the cached Isolate.
         event = new ServiceObject._fromMap(this, map);
-        _onIsolateShutdown(isolateId);
+        _onIsolateExit(isolateId);
       }
       assert(event != null);
       events.add(event);
@@ -426,11 +426,12 @@ abstract class VM extends ServiceObjectOwner {
           return;
         }
         var event = new ServiceObject._fromMap(owningIsolate, map);
+        owningIsolate._onEvent(event);
         events.add(event);
     });
   }
 
-  Isolate _onIsolateCreated(Map isolateMap) {
+  Isolate _onIsolateStart(Map isolateMap) {
     var isolateId = isolateMap['id'];
     assert(!_isolateCache.containsKey(isolateId));
     Isolate isolate = new ServiceObject._fromMap(this, isolateMap);
@@ -443,7 +444,7 @@ abstract class VM extends ServiceObjectOwner {
     return isolate;
   }
 
-  void _onIsolateShutdown(String isolateId) {
+  void _onIsolateExit(String isolateId) {
     assert(_isolateCache.containsKey(isolateId));
     _isolateCache.remove(isolateId);
     notifyPropertyChange(#isolates, true, false);
@@ -478,12 +479,12 @@ abstract class VM extends ServiceObjectOwner {
 
     // Process shutdown.
     for (var isolateId in shutdownIsolates) {
-      _onIsolateShutdown(isolateId);
+      _onIsolateExit(isolateId);
     }
 
     // Process creation.
     for (var isolateMap in createdIsolates) {
-      _onIsolateCreated(isolateMap);
+      _onIsolateStart(isolateMap);
     }
   }
 
@@ -505,6 +506,10 @@ abstract class VM extends ServiceObjectOwner {
       // We should never see an unknown isolate here.
       throw new UnimplementedError();
     }
+    var mapIsRef = _hasRef(map['type']);
+    if (!mapIsRef) {
+      isolate.update(map);
+    }
     return isolate;
   }
 
@@ -774,11 +779,23 @@ class Isolate extends ServiceObjectOwner with Coverage {
   @observable ObservableMap counters = new ObservableMap();
 
   @observable ServiceEvent pauseEvent = null;
-  bool get _isPaused => pauseEvent != null;
 
+  void _updateRunState() {
+    topFrame = (pauseEvent != null ? pauseEvent.topFrame : null);
+    paused = pauseEvent != null && pauseEvent.eventType != 'Resume';
+    running = (!paused && topFrame != null);
+    idle = (!paused && topFrame == null);
+    notifyPropertyChange(#topFrame, 0, 1);
+    notifyPropertyChange(#paused, 0, 1);
+    notifyPropertyChange(#running, 0, 1);
+    notifyPropertyChange(#idle, 0, 1);
+  }
+
+  @observable bool paused = false;
   @observable bool running = false;
   @observable bool idle = false;
   @observable bool loading = true;
+
   @observable bool ioEnabled = false;
 
   Map<String,ServiceObject> _cache = new Map<String,ServiceObject>();
@@ -844,7 +861,10 @@ class Isolate extends ServiceObjectOwner with Coverage {
     String mapId = map['id'];
     var obj = (mapId != null) ? _cache[mapId] : null;
     if (obj != null) {
-      // Consider calling update when map is not a reference.
+      var mapIsRef = _hasRef(map['type']);
+      if (!mapIsRef) {
+        obj.update(map);
+      }
       return obj;
     }
     // Build the object from the map directly.
@@ -862,12 +882,7 @@ class Isolate extends ServiceObjectOwner with Coverage {
 
   Future<ServiceObject> invokeRpc(String method, Map params) {
     return invokeRpcNoUpgrade(method, params).then((ObservableMap response) {
-      var obj = new ServiceObject._fromMap(this, response);
-      if ((obj != null) && obj.canCache) {
-        String objId = obj.id;
-        _cache.putIfAbsent(objId, () => obj);
-      }
-      return obj;
+      return getFromMap(response);
     });
   }
 
@@ -912,7 +927,7 @@ class Isolate extends ServiceObjectOwner with Coverage {
   @observable HeapSnapshot latestSnapshot;
   Completer<HeapSnapshot> _snapshotFetch;
 
-  void loadHeapSnapshot(ServiceEvent event) {
+  void _loadHeapSnapshot(ServiceEvent event) {
     latestSnapshot = new HeapSnapshot(this, event.data);
     _snapshotFetch.complete(latestSnapshot);
   }
@@ -940,7 +955,6 @@ class Isolate extends ServiceObjectOwner with Coverage {
     _loaded = true;
     loading = false;
 
-    reloadBreakpoints();
     _upgradeCollection(map, isolate);
     if (map['rootLib'] == null ||
         map['timers'] == null ||
@@ -952,11 +966,6 @@ class Isolate extends ServiceObjectOwner with Coverage {
     if (map['entry'] != null) {
       entry = map['entry'];
     }
-    if (map['topFrame'] != null) {
-      topFrame = map['topFrame'];
-    } else {
-      topFrame = null ;
-    }
 
     var countersMap = map['tagCounters'];
     if (countersMap != null) {
@@ -994,6 +1003,7 @@ class Isolate extends ServiceObjectOwner with Coverage {
     timers['dart'] = timerMap['time_dart_execution'];
 
     updateHeapsFromMap(map['heaps']);
+    _updateBreakpoints(map['breakpoints']);
 
     List features = map['features'];
     if (features != null) {
@@ -1003,10 +1013,8 @@ class Isolate extends ServiceObjectOwner with Coverage {
         }
       }
     }
-    // Isolate status
     pauseEvent = map['pauseEvent'];
-    running = (!_isPaused && map['topFrame'] != null);
-    idle = (!_isPaused && map['topFrame'] == null);
+    _updateRunState();
     error = map['error'];
 
     libraries.clear();
@@ -1023,71 +1031,78 @@ class Isolate extends ServiceObjectOwner with Coverage {
       });
   }
 
-  ObservableList<Breakpoint> breakpoints = new ObservableList();
+  ObservableMap<int, Breakpoint> breakpoints = new ObservableMap();
 
-  void _removeBreakpoint(Breakpoint bpt) {
-    var script = bpt.script;
-    var tokenPos = bpt.tokenPos;
-    assert(tokenPos != null);
-    if (script.loaded) {
-      var line = script.tokenToLine(tokenPos);
-      assert(line != null);
-      if (script.lines[line - 1] != null) {
-        assert(script.lines[line - 1].bpt == bpt);
-        script.lines[line - 1].bpt = null;
+  void _updateBreakpoints(List newBpts) {
+    // Build a map of new breakpoints.
+    var newBptMap = {};
+    newBpts.forEach((bpt) => (newBptMap[bpt.number] = bpt));
+
+    // Remove any old breakpoints which no longer exist.
+    List toRemove = [];
+    breakpoints.forEach((key, _) {
+      if (!newBptMap.containsKey(key)) {
+        toRemove.add(key);
       }
-    }
+    });
+    toRemove.forEach((key) => breakpoints.remove(key));
+
+    // Add all new breakpoints.
+    breakpoints.addAll(newBptMap);
   }
 
   void _addBreakpoint(Breakpoint bpt) {
-    var script = bpt.script;
-    var tokenPos = bpt.tokenPos;
-    assert(tokenPos != null);
-    if (script.loaded) {
-      var line = script.tokenToLine(tokenPos);
-      assert(line != null);
-      assert(script.lines[line - 1].bpt == null);
-      script.lines[line - 1].bpt = bpt;
-    } else {
-      // Load the script and then plop in the breakpoint.
-      script.load().then((_) {
-          _addBreakpoint(bpt);
-      });
-    }
+    breakpoints[bpt.number] = bpt;
   }
 
-  void _updateBreakpoints(ServiceMap newBreakpoints) {
-    // Remove all of the old breakpoints from the Script lines.
-    if (breakpoints != null) {
-      for (var bpt in breakpoints) {
-        _removeBreakpoint(bpt);
-      }
-    }
-    // Add all of the new breakpoints to the Script lines.
-    for (var bpt in newBreakpoints['breakpoints']) {
-      _addBreakpoint(bpt);
-    }
-    breakpoints.clear();
-    breakpoints.addAll(newBreakpoints['breakpoints']);
-
-    // Sort the breakpoints by breakpointNumber.
-    breakpoints.sort((a, b) => (a.number - b.number));
+  void _removeBreakpoint(Breakpoint bpt) {
+    breakpoints.remove(bpt.number);
+    bpt.remove();
   }
 
-  Future<ServiceObject> _inProgressReloadBpts;
+  void _onEvent(ServiceEvent event) {
+    switch(event.eventType) {
+      case 'IsolateStart':
+      case 'IsolateExit':
+        Logger.root.severe(
+            'Isolate lifecycle event should not be delivered to isolate');

[Cutch, 2015/03/05 16:30:57]

Throw here.

[turnidge, 2015/03/05 18:42:24]

Turned this into an assert.

+        break;
+
+      case 'BreakpointAdded':
+        _addBreakpoint(event.breakpoint);
+        break;
 
-  Future reloadBreakpoints() {
-    // TODO(turnidge): Can reusing the Future here ever cause us to
-    // get stale breakpoints?
-    if (_inProgressReloadBpts == null) {
-      _inProgressReloadBpts =
-          invokeRpc('getBreakpoints', {}).then((newBpts) {
-              _updateBreakpoints(newBpts);
-          }).whenComplete(() {
-              _inProgressReloadBpts = null;
-          });
+      case 'BreakpointResolved':
+        // Update occurs as side-effect of caching.
+        break;
+
+      case 'BreakpointRemoved':
+        _removeBreakpoint(event.breakpoint);
+        break;
+
+      case 'PauseStart':
+      case 'PauseExit':
+      case 'PauseBreakpoint':
+      case 'PauseInterrupted':
+      case 'PauseException':
+      case 'Resume':
+        pauseEvent = event;
+        _updateRunState();
+        break;
+
+      case '_Graph':
+        _loadHeapSnapshot(event);
+        break;
+
+      case 'GC':
+        // Ignore GC events for now.
+        break;
+
+      default:
+        // Log unrecognized events.
+        Logger.root.severe('Unrecognized event: $event');
+        break;
     }
-    return _inProgressReloadBpts;
   }
 
   Future<ServiceObject> addBreakpoint(Script script, int line) {
@@ -1107,41 +1122,18 @@ class Isolate extends ServiceObjectOwner with Coverage {
         // Unable to set a breakpoint at desired line.
         script.lines[line - 1].possibleBpt = false;
       }
-      // TODO(turnidge): Instead of reloading all of the breakpoints,
-      // rely on events to update the breakpoint list.
-      return reloadBreakpoints().then((_) {
-        return result;
-      });
+      return result;
     });
   }
 
   Future<ServiceObject> addBreakpointAtEntry(ServiceFunction function) {
     return invokeRpc('addBreakpointAtEntry',
-                     { 'functionId': function.id }).then((result) {
-        // TODO(turnidge): Instead of reloading all of the breakpoints,
-        // rely on events to update the breakpoint list.
-        return reloadBreakpoints().then((_) {
-            return result;
-        });
-      });
+                     { 'functionId': function.id });
   }
 
   Future removeBreakpoint(Breakpoint bpt) {
     return invokeRpc('removeBreakpoint',
-                     { 'breakpointId': bpt.id }).then((result) {
-        if (result is DartError) {
-          // TODO(turnidge): Handle this more gracefully.
-          Logger.root.severe(result.message);
-          return result;
-        }
-        if (pauseEvent != null &&
-            pauseEvent.breakpoint != null &&
-            (pauseEvent.breakpoint.id == bpt.id)) {
-          return isolate.reload();
-        } else {
-          return reloadBreakpoints();
-        }
-      });
+                     { 'breakpointId': bpt.id });
   }
 
   // TODO(turnidge): If the user invokes pause (or other rpcs) twice,
@@ -1319,14 +1311,10 @@ class ServiceMap extends ServiceObject implements ObservableMap {
 
   ServiceMap._empty(ServiceObjectOwner owner) : super._empty(owner);
 
-  void _upgradeValues() {
-    assert(owner != null);
-    _upgradeCollection(_map, owner);
-  }
-
   void _update(ObservableMap map, bool mapIsRef) {
     _loaded = !mapIsRef;
 
+    _upgradeCollection(map, owner);
     // TODO(turnidge): Currently _map.clear() prevents us from
     // upgrading an already upgraded submap.  Is clearing really the
     // right thing to do here?
@@ -1335,7 +1323,6 @@ class ServiceMap extends ServiceObject implements ObservableMap {
 
     name = _map['name'];
     vmName = (_map.containsKey('vmName') ? _map['vmName'] : name);
-    _upgradeValues();
   }
 
   // Forward Map interface calls.
@@ -1440,6 +1427,7 @@ class ServiceEvent extends ServiceObject {
 
   @observable String eventType;
   @observable Breakpoint breakpoint;
+  @observable ServiceMap topFrame;
   @observable ServiceMap exception;
   @observable ByteData data;
   @observable int count;
@@ -1453,6 +1441,9 @@ class ServiceEvent extends ServiceObject {
     if (map['breakpoint'] != null) {
       breakpoint = map['breakpoint'];
     }
+    if (map['topFrame'] != null) {
+      topFrame = map['topFrame'];
+    }
     if (map['exception'] != null) {
       exception = map['exception'];
     }
@@ -1465,8 +1456,12 @@ class ServiceEvent extends ServiceObject {
   }
 
   String toString() {
-    return 'ServiceEvent of type $eventType with '
-        '${data == null ? 0 : data.lengthInBytes} bytes of binary data';
+    if (data == null) {
+      return "ServiceEvent(owner='${owner.id}', type='${eventType}')";
+    } else {
+      return "ServiceEvent(owner='${owner.id}', type='${eventType}', "
+          "data.lengthInBytes=${data.lengthInBytes})";
+    }
   }
 }
 
@@ -1488,19 +1483,44 @@ class Breakpoint extends ServiceObject {
   // The breakpoint has been assigned to a final source location.
   @observable bool resolved;
 
-  // The breakpoint is active.
-  @observable bool enabled;
-
   void _update(ObservableMap map, bool mapIsRef) {
     _loaded = true;
     _upgradeCollection(map, owner);
 
+    var newNumber = map['breakpointNumber'];
+    var newScript = map['location']['script'];
+    var newTokenPos = map['location']['tokenPos'];
+
+    // number and script never change.
+    assert(number == null || number == newNumber);

[Cutch, 2015/03/05 16:30:57]

Wrap comparisons in ( ) here and elsewhere.

e.g. ((number == null) || (number == newNumber))

[turnidge, 2015/03/05 18:43:14]

Done.

+    assert(script == null || script == newScript);
+
     number = map['breakpointNumber'];
     script = map['location']['script'];
-    tokenPos = map['location']['tokenPos'];
-
     resolved = map['resolved'];
-    enabled = map['enabled'];
+    bool tokenPosChanged = tokenPos != newTokenPos;
+
+    if (script.loaded &&
+       tokenPos != null &&
+       tokenPosChanged) {
+      // The breakpoint has moved.  Remove it and add it later.
+      script._removeBreakpoint(this);
+    }
+
+    tokenPos = newTokenPos;
+    if (script.loaded && tokenPosChanged) {
+      script._addBreakpoint(this);
+    }
+  }
+
+  void remove() {
+    // Remove any references to this breakpoint.  It has been removed.
+    script._removeBreakpoint(this);
+    if (isolate.pauseEvent != null &&
+        isolate.pauseEvent.breakpoint != null &&
+        isolate.pauseEvent.breakpoint.id == id) {
+      isolate.pauseEvent.breakpoint = null;
+    }
   }
 
   String toString() {
@@ -1969,8 +1989,9 @@ class ScriptLine extends Observable {
   final int line;
   final String text;
   @observable int hits;
-  @observable Breakpoint bpt;
   @observable bool possibleBpt = true;
+  @observable bool breakpointResolved = false;
+  @observable Set<Breakpoint> breakpoints;
 
   bool get isBlank {
     // Compute isBlank on demand.
@@ -2015,13 +2036,22 @@ class ScriptLine extends Observable {
 
   ScriptLine(this.script, this.line, this.text) {
     possibleBpt = !_isTrivialLine(text);
+  }
 
-    // TODO(turnidge): This is not so efficient.  Consider improving.
-    for (var bpt in this.script.isolate.breakpoints) {
-      if (bpt.script == this.script &&
-          bpt.script.tokenToLine(bpt.tokenPos) == line) {
-        this.bpt = bpt;
-      }
+  void addBreakpoint(Breakpoint bpt) {
+    if (breakpoints == null) {
+      breakpoints = new Set<Breakpoint>();
+    }
+    breakpoints.add(bpt);
+    breakpointResolved = breakpointResolved || bpt.resolved;
+  }
+
+  void removeBreakpoint(Breakpoint bpt) {
+    assert(breakpoints != null && breakpoints.contains(bpt));
+    breakpoints.remove(bpt);
+    if (breakpoints.isEmpty) {
+      breakpoints = null;
+      breakpointResolved = false;
     }
   }
 }
@@ -2064,8 +2094,8 @@ class Script extends ServiceObject with Coverage {
     if (mapIsRef) {
       return;
     }
-    _processSource(map['source']);
     _parseTokenPosTable(map['tokenPosTable']);
+    _processSource(map['source']);
     owningLibrary = map['owningLibrary'];
   }
 
@@ -2145,6 +2175,12 @@ class Script extends ServiceObject with Coverage {
     for (var i = 0; i < sourceLines.length; i++) {
       lines.add(new ScriptLine(this, i + 1, sourceLines[i]));
     }
+    for (var bpt in isolate.breakpoints.values) {
+      if (bpt.script == this) {
+        _addBreakpoint(bpt);
+      }
+    }
+
     _applyHitsToLines();
     // Notify any Observers that this Script's state has changed.
     notifyChange(null);
@@ -2156,6 +2192,18 @@ class Script extends ServiceObject with Coverage {
       line.hits = hits;
     }
   }
+
+  void _addBreakpoint(Breakpoint bpt) {
+    var line = tokenToLine(bpt.tokenPos);
+    getLine(line).addBreakpoint(bpt);
+  }
+
+  void _removeBreakpoint(Breakpoint bpt) {
+    var line = tokenToLine(bpt.tokenPos);
+    if (line != null) {
+      getLine(line).removeBreakpoint(bpt);
+    }
+  }
 }
 
 class PcDescriptor extends Observable {