Skip to content

Revert "Make inspector weakly referencing the inspected objects." #128436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zanderso merged 1 commit into from
Jun 7, 2023
Merged

Revert "Make inspector weakly referencing the inspected objects." #128436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 17 additions & 99 deletions packages/flutter/lib/src/widgets/widget_inspector.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -680,39 +680,11 @@ typedef InspectorSelectionChangedCallback = void Function();

/// Structure to help reference count Dart objects referenced by a GUI tool
/// using [WidgetInspectorService].
///
/// Does not hold the object from garbage collection.
@visibleForTesting
class InspectorReferenceData {
/// Creates an instance of [InspectorReferenceData].
InspectorReferenceData(Object object, this.id) {
// These types are not supported by [WeakReference].
// See https://api.dart.dev/stable/3.0.2/dart-core/WeakReference-class.html
if (object is String || object is num || object is bool) {
_value = object;
return;
}

_ref = WeakReference<Object>(object);
}

WeakReference<Object>? _ref;

Object? _value;

/// The id of the object in the widget inspector records.
final String id;
class _InspectorReferenceData {
_InspectorReferenceData(this.object);

/// The number of times the object has been referenced.
final Object object;
int count = 1;

/// The value.
Object? get value {
if (_ref != null) {
return _ref!.target;
}
return _value;
}
}

// Production implementation of [WidgetInspectorService].
Expand Down Expand Up @@ -770,9 +742,9 @@ mixin WidgetInspectorService {
/// The VM service protocol does not keep alive object references so this
/// class needs to manually manage groups of objects that should be kept
/// alive.
final Map<String, Set<InspectorReferenceData>> _groups = <String, Set<InspectorReferenceData>>{};
final Map<String, InspectorReferenceData> _idToReferenceData = <String, InspectorReferenceData>{};
final WeakMap<Object, String> _objectToId = WeakMap<Object, String>();
final Map<String, Set<_InspectorReferenceData>> _groups = <String, Set<_InspectorReferenceData>>{};
final Map<String, _InspectorReferenceData> _idToReferenceData = <String, _InspectorReferenceData>{};
final Map<Object, String> _objectToId = Map<Object, String>.identity();
int _nextId = 0;

/// The pubRootDirectories that are currently configured for the widget inspector.
Expand Down Expand Up @@ -1298,22 +1270,20 @@ mixin WidgetInspectorService {
/// references from a different group.
@protected
void disposeGroup(String name) {
final Set<InspectorReferenceData>? references = _groups.remove(name);
final Set<_InspectorReferenceData>? references = _groups.remove(name);
if (references == null) {
return;
}
references.forEach(_decrementReferenceCount);
}

void _decrementReferenceCount(InspectorReferenceData reference) {
void _decrementReferenceCount(_InspectorReferenceData reference) {
reference.count -= 1;
assert(reference.count >= 0);
if (reference.count == 0) {
final Object? value = reference.value;
if (value != null) {
_objectToId.remove(value);
}
_idToReferenceData.remove(reference.id);
final String? id = _objectToId.remove(reference.object);
assert(id != null);
_idToReferenceData.remove(id);
}
}

Expand All @@ -1325,15 +1295,14 @@ mixin WidgetInspectorService {
return null;
}

final Set<InspectorReferenceData> group = _groups.putIfAbsent(groupName, () => Set<InspectorReferenceData>.identity());
final Set<_InspectorReferenceData> group = _groups.putIfAbsent(groupName, () => Set<_InspectorReferenceData>.identity());
String? id = _objectToId[object];
InspectorReferenceData referenceData;
_InspectorReferenceData referenceData;
if (id == null) {
// TODO(polina-c): comment here why we increase memory footprint by the prefix 'inspector-'.
id = 'inspector-$_nextId';
_nextId += 1;
_objectToId[object] = id;
referenceData = InspectorReferenceData(object, id);
referenceData = _InspectorReferenceData(object);
_idToReferenceData[id] = referenceData;
group.add(referenceData);
} else {
Expand Down Expand Up @@ -1363,11 +1332,11 @@ mixin WidgetInspectorService {
return null;
}

final InspectorReferenceData? data = _idToReferenceData[id];
final _InspectorReferenceData? data = _idToReferenceData[id];
if (data == null) {
throw FlutterError.fromParts(<DiagnosticsNode>[ErrorSummary('Id does not exist.')]);
}
return data.value;
return data.object;
}

/// Returns the object to introspect to determine the source location of an
Expand Down Expand Up @@ -1399,7 +1368,7 @@ mixin WidgetInspectorService {
return;
}

final InspectorReferenceData? referenceData = _idToReferenceData[id];
final _InspectorReferenceData? referenceData = _idToReferenceData[id];
if (referenceData == null) {
throw FlutterError.fromParts(<DiagnosticsNode>[ErrorSummary('Id does not exist')]);
}
Expand Down Expand Up @@ -3744,54 +3713,3 @@ class _WidgetFactory {
// recognize the annotation.
// ignore: library_private_types_in_public_api
const _WidgetFactory widgetFactory = _WidgetFactory();

/// Does not hold keys from garbage collection.
@visibleForTesting
class WeakMap<K, V> {
Expando<Object> _objects = Expando<Object>();

/// Strings, numbers, booleans.
final Map<K, V> _primitives = <K, V>{};

bool _isPrimitive(Object? key) {
return key == null || key is String || key is num || key is bool;
}

/// Returns the value for the given [key] or null if [key] is not in the map
/// or garbage collected.
///
/// Does not support records to act as keys.
V? operator [](K key){
if (_isPrimitive(key)) {
return _primitives[key];
} else {
return _objects[key!] as V?;
}
}

/// Associates the [key] with the given [value].
void operator []=(K key, V value){
if (_isPrimitive(key)) {
_primitives[key] = value;
} else {
_objects[key!] = value;
}
}

/// Removes the value for the given [key] from the map.
V? remove(K key) {
if (_isPrimitive(key)) {
return _primitives.remove(key);
} else {
final V? result = _objects[key!] as V?;
_objects[key] = null;
return result;
}
}

/// Removes all pairs from the map.
void clear() {
_objects = Expando<Object>();
_primitives.clear();
}
}
83 changes: 4 additions & 79 deletions packages/flutter/test/widgets/widget_inspector_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@
@TestOn('!chrome')
library;

import 'dart:async';
import 'dart:convert';
import 'dart:developer';
import 'dart:math';
import 'dart:ui' as ui;

Expand Down Expand Up @@ -242,67 +240,7 @@ extension TextFromString on String {
}
}

/// Forces garbage collection by aggressive memory allocation.
Future<void> _forceGC() async {
const Duration timeout = Duration(seconds: 5);
const int gcCycles = 3; // 1 should be enough, but we do 3 to make sure test is not flaky.
final Stopwatch stopwatch = Stopwatch()..start();
final int barrier = reachabilityBarrier;

final List<List<DateTime>> storage = <List<DateTime>>[];

void allocateMemory() {
storage.add(Iterable<DateTime>.generate(10000, (_) => DateTime.now()).toList());
if (storage.length > 100) {
storage.removeAt(0);
}
}

while (reachabilityBarrier < barrier + gcCycles) {
if (stopwatch.elapsed > timeout) {
throw TimeoutException('forceGC timed out', timeout);
}
await Future<void>.delayed(Duration.zero);
allocateMemory();
}
}


final List<Object> _weakValueTests = <Object>[1, 1.0, 'hello', true, false, Object(), <int>[3, 4], DateTime(2023)];

void main() {
group('$InspectorReferenceData', (){
for (final Object item in _weakValueTests) {
test('can be created for any type but $Record, $item', () async {
final InspectorReferenceData weakValue = InspectorReferenceData(item, 'id');
expect(weakValue.value, item);
});
}

test('throws for $Record', () async {
expect(()=> InspectorReferenceData((1, 2), 'id'), throwsA(isA<ArgumentError>()));
});
});

group('$WeakMap', (){
for (final Object item in _weakValueTests) {
test('assigns and removes value, $item', () async {
final WeakMap<Object, Object> weakMap = WeakMap<Object, Object>();
weakMap[item] = 1;
expect(weakMap[item], 1);
expect(weakMap.remove(item), 1);
expect(weakMap[item], null);
});
}

for (final Object item in _weakValueTests) {
test('returns null for absent value, $item', () async {
final WeakMap<Object, Object> weakMap = WeakMap<Object, Object>();
expect(weakMap[item], null);
});
}
});

_TestWidgetInspectorService.runTests();
}

Expand All @@ -323,19 +261,6 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
}
});

test('WidgetInspector does not hold objects from GC', () async {
List<DateTime>? someObject = <DateTime>[DateTime.now(), DateTime.now()];
final String? id = service.toId(someObject, 'group_name');

expect(id, isNotNull);

final WeakReference<Object> ref = WeakReference<Object>(someObject);
someObject = null;
await _forceGC();

expect(ref.target, null);
});

testWidgets('WidgetInspector smoke test', (WidgetTester tester) async {
// This is a smoke test to verify that adding the inspector doesn't crash.
await tester.pumpWidget(
Expand Down Expand Up @@ -3820,7 +3745,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
_CreationLocation location = knownLocations[id]!;
expect(location.file, equals(file));
// ClockText widget.
expect(location.line, equals(57));
expect(location.line, equals(55));
expect(location.column, equals(9));
expect(location.name, equals('ClockText'));
expect(count, equals(1));
Expand All @@ -3830,7 +3755,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
location = knownLocations[id]!;
expect(location.file, equals(file));
// Text widget in _ClockTextState build method.
expect(location.line, equals(95));
expect(location.line, equals(93));
expect(location.column, equals(12));
expect(location.name, equals('Text'));
expect(count, equals(1));
Expand All @@ -3857,7 +3782,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
location = knownLocations[id]!;
expect(location.file, equals(file));
// ClockText widget.
expect(location.line, equals(57));
expect(location.line, equals(55));
expect(location.column, equals(9));
expect(location.name, equals('ClockText'));
expect(count, equals(3)); // 3 clock widget instances rebuilt.
Expand All @@ -3867,7 +3792,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
location = knownLocations[id]!;
expect(location.file, equals(file));
// Text widget in _ClockTextState build method.
expect(location.line, equals(95));
expect(location.line, equals(93));
expect(location.column, equals(12));
expect(location.name, equals('Text'));
expect(count, equals(3)); // 3 clock widget instances rebuilt.
Expand Down