Major rework of vm service events.

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

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 part of service;
 
 /// A [ServiceObject] represents a persistent object within the vm.
 abstract class ServiceObject extends Observable {
   static int LexicalSortName(ServiceObject o1, ServiceObject o2) {
     return o1.name.compareTo(o2.name);
@@ skipping 346 matching lines @@
   }
 
   final StreamController<ServiceException> exceptions =
       new StreamController.broadcast();
   final StreamController<ServiceError> errors =
       new StreamController.broadcast();
   final StreamController<ServiceEvent> events =
       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) {
     var map;
     try {
       map = _parseJSON(response);
       assert(!map.containsKey('_data'));
       if (data != null) {
         map['_data'] = data;
       }
     } catch (e, st) {
       Logger.root.severe('Ignoring malformed event response: ${response}');
       return;
     }
     if (map['type'] != 'ServiceEvent') {
       Logger.root.severe(
           "Expected 'ServiceEvent' but found '${map['type']}'");
       return;
     }
 
     var eventType = map['eventType'];
 
     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);
       return;
     }
 
     // Extract the owning isolate from the event itself.
     String owningIsolateId = map['isolate']['id'];
     getIsolate(owningIsolateId).then((owningIsolate) {
         if (owningIsolate == null) {
           // TODO(koda): Do we care about GC events in VM isolate?
           Logger.root.severe('Ignoring event with unknown isolate id: '
                              '$owningIsolateId');
           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);
     _isolateCache[isolateId] = isolate;
     notifyPropertyChange(#isolates, true, false);
     // Eagerly load the isolate.
     isolate.load().catchError((e) {
       Logger.root.info('Eagerly loading an isolate failed: $e');
     });
     return isolate;
   }
 
-  void _onIsolateShutdown(String isolateId) {
+  void _onIsolateExit(String isolateId) {
     assert(_isolateCache.containsKey(isolateId));
     _isolateCache.remove(isolateId);
     notifyPropertyChange(#isolates, true, false);
   }
 
   void _updateIsolatesFromList(List isolateList) {
     var shutdownIsolates = <String>[];
     var createdIsolates = <Map>[];
     var isolateStillExists = <String, bool>{};
 
@@ skipping 14 matching lines @@
 
     // Find shutdown isolates.
     isolateStillExists.forEach((isolateId, exists) {
       if (!exists) {
         shutdownIsolates.add(isolateId);
       }
     });
 
     // Process shutdown.
     for (var isolateId in shutdownIsolates) {
-      _onIsolateShutdown(isolateId);
+      _onIsolateExit(isolateId);
     }
 
     // Process creation.
     for (var isolateMap in createdIsolates) {
-      _onIsolateCreated(isolateMap);
+      _onIsolateStart(isolateMap);
     }
   }
 
   static final String _isolateIdPrefix = 'isolates/';
 
   ServiceObject getFromMap(ObservableMap map) {
     if (map == null) {
       return null;
     }
     String id = map['id'];
     if (!id.startsWith(_isolateIdPrefix)) {
       // Currently the VM only supports upgrading Isolate ServiceObjects.
       throw new UnimplementedError();
     }
 
     // Check cache.
     var isolate = _isolateCache[id];
     if (isolate == null) {
       // We should never see an unknown isolate here.
       throw new UnimplementedError();
     }
+    var mapIsRef = _hasRef(map['type']);
+    if (!mapIsRef) {
+      isolate.update(map);
+    }
     return isolate;
   }
 
   // Note that this function does not reload the isolate if it found
   // in the cache.
   Future<ServiceObject> getIsolate(String isolateId) {
     if (!loaded) {
       // Trigger a VM load, then get the isolate.
       return load().then((_) => getIsolate(isolateId)).catchError(_ignoreError);
     }
@@ skipping 249 matching lines @@
 
 }
 
 /// State for a running isolate.
 class Isolate extends ServiceObjectOwner with Coverage {
   @reflectable VM get vm => owner;
   @reflectable Isolate get isolate => this;
   @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>();
   final TagProfile tagProfile = new TagProfile(20);
 
   Isolate._empty(ServiceObjectOwner owner) : super._empty(owner) {
     assert(owner is VM);
   }
 
   void resetCachedProfileData() {
@@ skipping 45 matching lines @@
     return new Future.value(objectClass);
   }
 
   ServiceObject getFromMap(ObservableMap map) {
     if (map == null) {
       return null;
     }
     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.
     obj = new ServiceObject._fromMap(this, map);
     if ((obj != null) && obj.canCache) {
       _cache[mapId] = obj;
     }
     return obj;
   }
 
   Future<ObservableMap> invokeRpcNoUpgrade(String method, Map params) {
     params['isolateId'] = id;
     return vm.invokeRpcNoUpgrade(method, params);
   }
 
   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);
     });
   }
 
   Future<ServiceObject> getObject(String objectId) {
     assert(objectId != null && objectId != '');
     var obj = _cache[objectId];
     if (obj != null) {
       return obj.reload();
     }
     Map params = {
@@ skipping 24 matching lines @@
 
   final HeapSpace newSpace = new HeapSpace();
   final HeapSpace oldSpace = new HeapSpace();
 
   @observable String fileAndLine;
 
   @observable DartError error;
   @observable HeapSnapshot latestSnapshot;
   Completer<HeapSnapshot> _snapshotFetch;
 
-  void loadHeapSnapshot(ServiceEvent event) {
+  void _loadHeapSnapshot(ServiceEvent event) {
     latestSnapshot = new HeapSnapshot(this, event.data);
     _snapshotFetch.complete(latestSnapshot);
   }
 
   Future<HeapSnapshot> fetchHeapSnapshot() {
     if (_snapshotFetch == null || _snapshotFetch.isCompleted) {
       _snapshotFetch = new Completer<HeapSnapshot>();
       isolate.invokeRpcNoUpgrade('requestHeapSnapshot', {});
     }
     return _snapshotFetch.future;
   }
 
   void updateHeapsFromMap(ObservableMap map) {
     newSpace.update(map['new']);
     oldSpace.update(map['old']);
   }
 
   void _update(ObservableMap map, bool mapIsRef) {
     mainPort = map['mainPort'];
     name = map['name'];
     vmName = map['name'];
     if (mapIsRef) {
       return;
     }
     _loaded = true;
     loading = false;
 
-    reloadBreakpoints();
     _upgradeCollection(map, isolate);
     if (map['rootLib'] == null ||
         map['timers'] == null ||
         map['heaps'] == null) {
       Logger.root.severe("Malformed 'Isolate' response: $map");
       return;
     }
     rootLib = map['rootLib'];
     if (map['entry'] != null) {
       entry = map['entry'];
     }
-    if (map['topFrame'] != null) {
-      topFrame = map['topFrame'];
-    } else {
-      topFrame = null ;
-    }
 
     var countersMap = map['tagCounters'];
     if (countersMap != null) {
       var names = countersMap['names'];
       var counts = countersMap['counters'];
       assert(names.length == counts.length);
       var sum = 0;
       for (var i = 0; i < counts.length; i++) {
         sum += counts[i];
       }
@@ skipping 17 matching lines @@
     timers['total'] = timerMap['time_total_runtime'];
     timers['compile'] = timerMap['time_compilation'];
     timers['gc'] = 0.0;  // TODO(turnidge): Export this from VM.
     timers['init'] = (timerMap['time_script_loading'] +
                       timerMap['time_creating_snapshot'] +
                       timerMap['time_isolate_initialization'] +
                       timerMap['time_bootstrap']);
     timers['dart'] = timerMap['time_dart_execution'];
 
     updateHeapsFromMap(map['heaps']);
+    _updateBreakpoints(map['breakpoints']);
 
     List features = map['features'];
     if (features != null) {
       for (var feature in features) {
         if (feature == 'io') {
           ioEnabled = true;
         }
       }
     }
-    // Isolate status
     pauseEvent = map['pauseEvent'];
-    running = (!_isPaused && map['topFrame'] != null);
-    idle = (!_isPaused && map['topFrame'] == null);
+    _updateRunState();
     error = map['error'];
 
     libraries.clear();
     libraries.addAll(map['libraries']);
     libraries.sort(ServiceObject.LexicalSortName);
   }
 
   Future<TagProfile> updateTagProfile() {
     return isolate.invokeRpcNoUpgrade('getTagProfile', {}).then(
       (ObservableMap map) {
         var seconds = new DateTime.now().millisecondsSinceEpoch / 1000.0;
         tagProfile._processTagProfile(seconds, map);
         return tagProfile;
       });
   }
 
-  ObservableList<Breakpoint> breakpoints = new ObservableList();
+  ObservableMap<int, Breakpoint> breakpoints = new ObservableMap();
+
+  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) {
+    breakpoints[bpt.number] = bpt;
+  }
 
   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;
-      }
+    breakpoints.remove(bpt.number);
+    bpt.remove();
+  }
+
+  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;
+
+      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;
     }
   }
 
-  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);
-      });
-    }
-  }
-
-  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));
-  }
-
-  Future<ServiceObject> _inProgressReloadBpts;
-
-  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;
-          });
-    }
-    return _inProgressReloadBpts;
-  }
-
   Future<ServiceObject> addBreakpoint(Script script, int line) {
     // TODO(turnidge): Pass line as an int instead of a string.
     Map params = {
       'scriptId': script.id,
       'line': '$line',
     };
     return invokeRpc('addBreakpoint', params).then((result) {
       if (result is DartError) {
         return result;
       }
       Breakpoint bpt = result;
       if (bpt.resolved &&
           script.loaded &&
           script.tokenToLine(result.tokenPos) != line) {
         // 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,
   // they could get a race.  Consider returning an "in progress"
   // future to avoid this.
   Future pause() {
     return invokeRpc('pause', {}).then((result) {
         if (result is DartError) {
           // TODO(turnidge): Handle this more gracefully.
           Logger.root.severe(result.message);
@@ skipping 157 matching lines @@
   bool get canCache {
     return (_type == 'Class' ||
             _type == 'Function' ||
             _type == 'Field') &&
            !_id.startsWith(objectIdRingPrefix);
   }
   bool get immutable => false;
 
   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?
     _map.clear();
     _map.addAll(map);
 
     name = _map['name'];
     vmName = (_map.containsKey('vmName') ? _map['vmName'] : name);
-    _upgradeValues();
   }
 
   // Forward Map interface calls.
   void addAll(Map other) => _map.addAll(other);
   void clear() => _map.clear();
   bool containsValue(v) => _map.containsValue(v);
   bool containsKey(k) => _map.containsKey(k);
   void forEach(Function f) => _map.forEach(f);
   putIfAbsent(key, Function ifAbsent) => _map.putIfAbsent(key, ifAbsent);
   void remove(key) => _map.remove(key);
@@ skipping 84 matching lines @@
 /// A [ServiceEvent] is an asynchronous event notification from the vm.
 class ServiceEvent extends ServiceObject {
   ServiceEvent._empty(ServiceObjectOwner owner) : super._empty(owner);
 
   ServiceEvent.vmDisconencted() : super._empty(null) {
     eventType = 'VMDisconnected';
   }
 
   @observable String eventType;
   @observable Breakpoint breakpoint;
+  @observable ServiceMap topFrame;
   @observable ServiceMap exception;
   @observable ByteData data;
   @observable int count;
 
   void _update(ObservableMap map, bool mapIsRef) {
     _loaded = true;
     _upgradeCollection(map, owner);
     eventType = map['eventType'];
     name = 'ServiceEvent $eventType';
     vmName = name;
     if (map['breakpoint'] != null) {
       breakpoint = map['breakpoint'];
     }
+    if (map['topFrame'] != null) {
+      topFrame = map['topFrame'];
+    }
     if (map['exception'] != null) {
       exception = map['exception'];
     }
     if (map['_data'] != null) {
       data = map['_data'];
     }
     if (map['count'] != null) {
       count = map['count'];
     }
   }
 
   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})";
+    }
   }
 }
 
 class Breakpoint extends ServiceObject {
   Breakpoint._empty(ServiceObjectOwner owner) : super._empty(owner);
 
   // TODO(turnidge): Add state to track if a breakpoint has been
   // removed from the program.  Remove from the cache when deleted.
   bool get canCache => true;
   bool get immutable => false;
 
   // A unique integer identifier for this breakpoint.
   @observable int number;
 
   // Source location information.
   @observable Script script;
   @observable int tokenPos;
 
   // 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'];
+    bool tokenPosChanged = tokenPos != newTokenPos;
 
-    resolved = map['resolved'];
-    enabled = map['enabled'];
+    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() {
     if (number != null) {
       return 'Breakpoint ${number} at ${script.name}(token:${tokenPos})';
     } else {
       return 'Uninitialized breakpoint';
     }
   }
 }
@@ skipping 448 matching lines @@
 
   String toString() => 'Field(${owner.name}.$name)';
 }
 
 
 class ScriptLine extends Observable {
   final Script script;
   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.
     if (_isBlank == null) {
       _isBlank = text.trim().isEmpty;
     }
     return _isBlank;
   }
   bool _isBlank;
 
@@ skipping 24 matching lines @@
         if (!_isTrivialToken(token)) {
           return false;
         }
       }
     }
     return true;
   }
 
   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;
     }
   }
 }
 
 class Script extends ServiceObject with Coverage {
   final lines = new ObservableList<ScriptLine>();
   final _hits = new Map<int, int>();
   @observable String kind;
   @observable int firstTokenPos;
   @observable int lastTokenPos;
@@ skipping 22 matching lines @@
   void _update(ObservableMap map, bool mapIsRef) {
     _upgradeCollection(map, isolate);
     kind = map['kind'];
     _url = map['name'];
     _shortUrl = _url.substring(_url.lastIndexOf('/') + 1);
     name = _shortUrl;
     vmName = _url;
     if (mapIsRef) {
       return;
     }
+    _parseTokenPosTable(map['tokenPosTable']);
     _processSource(map['source']);
-    _parseTokenPosTable(map['tokenPosTable']);
     owningLibrary = map['owningLibrary'];
   }
 
   void _parseTokenPosTable(List<List<int>> table) {
     if (table == null) {
       return;
     }
     _tokenToLine.clear();
     _tokenToCol.clear();
     firstTokenPos = null;
@@ skipping 59 matching lines @@
     if (sourceLines.length == 0) {
       return;
     }
     // We have the source to the script. This is now loaded.
     _loaded = true;
     lines.clear();
     Logger.root.info('Adding ${sourceLines.length} source lines for ${_url}');
     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);
   }
 
   void _applyHitsToLines() {
     for (var line in lines) {
       var hits = _hits[line.line];
       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 {
   final int pcOffset;
   @reflectable final int deoptId;
   @reflectable final int tokenPos;
   @reflectable final int tryIndex;
   @reflectable final String kind;
   @observable Script script;
   @observable String formattedLine;
@@ skipping 663 matching lines @@
     var v = list[i];
     if ((v is ObservableMap) && _isServiceMap(v)) {
       list[i] = owner.getFromMap(v);
     } else if (v is ObservableList) {
       _upgradeObservableList(v, owner);
     } else if (v is ObservableMap) {
       _upgradeObservableMap(v, owner);
     }
   }
 }