Skip to content

Commit

Permalink
Next attempt to make inspector weakly referencing the inspected objec…
Browse files Browse the repository at this point in the history
…ts. (#129962)
  • Loading branch information
polina-c committed Jul 6, 2023
1 parent 4f6c887 commit bff6b93
Show file tree
Hide file tree
Showing 5 changed files with 147 additions and 25 deletions.
2 changes: 2 additions & 0 deletions packages/flutter/lib/src/foundation/diagnostics.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3292,6 +3292,8 @@ mixin Diagnosticable {
/// {@end-tool}
///
/// Used by [toDiagnosticsNode] and [toString].
///
/// Do not add values, that have lifetime shorter than the object.
@protected
@mustCallSuper
void debugFillProperties(DiagnosticPropertiesBuilder properties) { }
Expand Down
2 changes: 1 addition & 1 deletion packages/flutter/lib/src/widgets/framework.dart
Original file line number Diff line number Diff line change
Expand Up @@ -4794,7 +4794,7 @@ abstract class Element extends DiagnosticableTree implements BuildContext {
final List<DiagnosticsNode> diagnosticsDependencies = sortedDependencies
.map((InheritedElement element) => element.widget.toDiagnosticsNode(style: DiagnosticsTreeStyle.sparse))
.toList();
properties.add(DiagnosticsProperty<List<DiagnosticsNode>>('dependencies', diagnosticsDependencies));
properties.add(DiagnosticsProperty<Set<InheritedElement>>('dependencies', deps, description: diagnosticsDependencies.toString()));
}
}

Expand Down
117 changes: 100 additions & 17 deletions packages/flutter/lib/src/widgets/widget_inspector.dart
Original file line number Diff line number Diff line change
Expand Up @@ -680,11 +680,39 @@ typedef InspectorSelectionChangedCallback = void Function();

/// Structure to help reference count Dart objects referenced by a GUI tool
/// using [WidgetInspectorService].
class _InspectorReferenceData {
_InspectorReferenceData(this.object);
///
/// 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;

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

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

// Production implementation of [WidgetInspectorService].
Expand Down Expand Up @@ -742,9 +770,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 Map<Object, String> _objectToId = Map<Object, String>.identity();
final Map<String, Set<InspectorReferenceData>> _groups = <String, Set<InspectorReferenceData>>{};
final Map<String, InspectorReferenceData> _idToReferenceData = <String, InspectorReferenceData>{};
final WeakMap<Object, String> _objectToId = WeakMap<Object, String>();
int _nextId = 0;

/// The pubRootDirectories that are currently configured for the widget inspector.
Expand Down Expand Up @@ -1270,20 +1298,22 @@ 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 String? id = _objectToId.remove(reference.object);
assert(id != null);
_idToReferenceData.remove(id);
final Object? value = reference.value;
if (value != null) {
_objectToId.remove(value);
}
_idToReferenceData.remove(reference.id);
}
}

Expand All @@ -1295,14 +1325,16 @@ 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-'.
// https://github.com/flutter/devtools/issues/5995
id = 'inspector-$_nextId';
_nextId += 1;
_objectToId[object] = id;
referenceData = _InspectorReferenceData(object);
referenceData = InspectorReferenceData(object, id);
_idToReferenceData[id] = referenceData;
group.add(referenceData);
} else {
Expand Down Expand Up @@ -1332,11 +1364,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.object;
return data.value;
}

/// Returns the object to introspect to determine the source location of an
Expand Down Expand Up @@ -1368,7 +1400,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 @@ -3731,3 +3763,54 @@ 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();
}
}
8 changes: 4 additions & 4 deletions packages/flutter/test/widgets/framework_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1592,13 +1592,13 @@ void main() {
builder.properties.any((DiagnosticsNode property) => property.name == 'dependencies' && property.value != null),
isTrue,
);
final DiagnosticsProperty<List<DiagnosticsNode>> dependenciesProperty =
builder.properties.firstWhere((DiagnosticsNode property) => property.name == 'dependencies') as DiagnosticsProperty<List<DiagnosticsNode>>;
final DiagnosticsProperty<Set<InheritedElement>> dependenciesProperty =
builder.properties.firstWhere((DiagnosticsNode property) => property.name == 'dependencies') as DiagnosticsProperty<Set<InheritedElement>>;
expect(dependenciesProperty, isNotNull);

final List<DiagnosticsNode> dependencies = dependenciesProperty.value!;
final Set<InheritedElement> dependencies = dependenciesProperty.value!;
expect(dependencies.length, equals(3));
expect(dependencies.toString(), '[ButtonBarTheme, Directionality, FocusTraversalOrder]');
expect(dependenciesProperty.toDescription(), '[ButtonBarTheme, Directionality, FocusTraversalOrder]');
});

testWidgets('BuildOwner.globalKeyCount keeps track of in-use global keys', (WidgetTester tester) async {
Expand Down
43 changes: 40 additions & 3 deletions packages/flutter/test/widgets/widget_inspector_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -240,7 +240,41 @@ extension TextFromString on String {
}
}

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 Down Expand Up @@ -2184,7 +2218,8 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
final List<DiagnosticsNode> expectedProperties = element.toDiagnosticsNode().getProperties();
final Iterable<Object?> propertyValues = expectedProperties.map((DiagnosticsNode e) => e.value.toString());
for (final Map<String, Object?> propertyJson in propertiesJson.cast<Map<String, Object?>>()) {
final String property = service.toObject(propertyJson['valueId']! as String)!.toString();
final String id = propertyJson['valueId']! as String;
final String property = service.toObject(id)!.toString();
expect(propertyValues, contains(property));
}
}
Expand Down Expand Up @@ -2227,7 +2262,8 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
final List<DiagnosticsNode> expectedProperties = element.toDiagnosticsNode().getProperties();
final Iterable<Object?> propertyValues = expectedProperties.map((DiagnosticsNode e) => e.value.toString());
for (final Map<String, Object?> propertyJson in propertiesJson.cast<Map<String, Object?>>()) {
final String property = service.toObject(propertyJson['valueId']! as String)!.toString();
final String id = propertyJson['valueId']! as String;
final String property = service.toObject(id)!.toString();
expect(propertyValues, contains(property));
}
}
Expand Down Expand Up @@ -2283,7 +2319,6 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {

testWidgets('ext.flutter.inspector.getRootWidgetSummaryTree', (WidgetTester tester) async {
const String group = 'test-group';

await tester.pumpWidget(
const Directionality(
textDirection: TextDirection.ltr,
Expand All @@ -2296,6 +2331,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
),
),
);

final Element elementA = find.text('a').evaluate().first;

service.disposeAllGroups();
Expand All @@ -2311,6 +2347,7 @@ class _TestWidgetInspectorService extends TestWidgetInspectorService {
WidgetInspectorServiceExtensions.getRootWidgetSummaryTree.name,
<String, String>{'objectGroup': group},
))! as Map<String, Object?>;

// We haven't yet properly specified which directories are summary tree
// directories so we get an empty tree other than the root that is always
// included.
Expand Down

0 comments on commit bff6b93

Please sign in to comment.