From b7a3d730237119aa6aa9dedec5e866426094d677 Mon Sep 17 00:00:00 2001 From: Polina Cherkasova Date: Wed, 17 Apr 2024 15:30:06 -0700 Subject: [PATCH] Refactor chart data. (#7609) 1. Remove live vs offline data from chart memory timeline. 2. Factor out chart data to separate class. No functional changes. --- .../memory/framework/memory_controller.dart | 4 +- .../panes/chart/controller/DEPENDENCIES.md | 1 + .../panes/chart/controller/chart_data.dart | 41 +++++ .../controller/chart_pane_controller.dart | 69 +++----- .../chart/widgets/chart_control_pane.dart | 6 +- .../panes/chart/widgets/chart_pane.dart | 6 +- .../chart/widgets/interval_dropdown.dart | 4 +- .../shared/primitives/memory_timeline.dart | 152 ++++++------------ .../shared/json_to_service_cache_test.dart | 66 ++++---- .../test_infra/scenes/memory/default.dart | 4 +- 10 files changed, 159 insertions(+), 194 deletions(-) create mode 100644 packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_data.dart diff --git a/packages/devtools_app/lib/src/screens/memory/framework/memory_controller.dart b/packages/devtools_app/lib/src/screens/memory/framework/memory_controller.dart index 9de3f0fb7a5..be98bf170ec 100644 --- a/packages/devtools_app/lib/src/screens/memory/framework/memory_controller.dart +++ b/packages/devtools_app/lib/src/screens/memory/framework/memory_controller.dart @@ -125,13 +125,13 @@ class MemoryController extends DisposableController diff = diffPaneController ?? offlineData?.diff ?? DiffPaneController( - loader: HeapGraphLoaderRuntime(chart.memoryTimeline), + loader: HeapGraphLoaderRuntime(chart.data.timeline), ); profile = profilePaneController ?? offlineData?.profile ?? ProfilePaneController(); control = MemoryControlPaneController( - chart.memoryTimeline, + chart.data.timeline, isChartVisible: chart.isChartVisible, exportData: exportData, ); diff --git a/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/DEPENDENCIES.md b/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/DEPENDENCIES.md index 8860261144b..54e3facbf2c 100644 --- a/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/DEPENDENCIES.md +++ b/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/DEPENDENCIES.md @@ -8,6 +8,7 @@ flowchart TD; chart_connection.dart-->memory_tracker.dart; chart_pane_controller.dart-->android_chart_controller.dart; chart_pane_controller.dart-->chart_connection.dart; +chart_pane_controller.dart-->chart_data.dart; chart_pane_controller.dart-->event_chart_controller.dart; chart_pane_controller.dart-->vm_chart_controller.dart; ``` diff --git a/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_data.dart b/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_data.dart new file mode 100644 index 00000000000..9ca37dd0e79 --- /dev/null +++ b/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_data.dart @@ -0,0 +1,41 @@ +// Copyright 2024 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'package:flutter/foundation.dart'; + +import '../../../../../shared/primitives/simple_items.dart'; +import '../../../shared/primitives/memory_timeline.dart'; +import '../data/primitives.dart'; + +/// Chart data, that should be saved when transferred to offline data mode. +class ChartData { + ChartData({required this.isDeviceAndroid}); + + /// Wether device is android, if [mode] is not [DevToolsMode.connected]. + /// + /// If [mode] is [DevToolsMode.connected], this value is null + /// and chart visibility should be detected based on connected app. + final bool? isDeviceAndroid; + + final MemoryTimeline timeline = MemoryTimeline(); + + /// Default is to display default tick width based on width of chart of the collected + /// data in the chart. + ChartInterval get displayInterval => _displayInterval.value; + final _displayInterval = + ValueNotifier(ChartInterval.theDefault); + set displayInterval(ChartInterval interval) { + _displayInterval.value = interval; + } + + ValueListenable get isLegendVisible => _legendVisibleNotifier; + final _legendVisibleNotifier = ValueNotifier(true); + bool toggleLegendVisibility() => + _legendVisibleNotifier.value = !_legendVisibleNotifier.value; + + void dispose() { + _displayInterval.dispose(); + _legendVisibleNotifier.dispose(); + } +} diff --git a/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_pane_controller.dart b/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_pane_controller.dart index d521eb2d6c7..22de41ec341 100644 --- a/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_pane_controller.dart +++ b/packages/devtools_app/lib/src/screens/memory/panes/chart/controller/chart_pane_controller.dart @@ -9,20 +9,20 @@ import 'package:flutter/foundation.dart'; import '../../../../../shared/globals.dart'; import '../../../../../shared/primitives/simple_items.dart'; -import '../../../shared/primitives/memory_timeline.dart'; -import '../data/primitives.dart'; import 'android_chart_controller.dart'; import 'chart_connection.dart'; +import 'chart_data.dart'; import 'event_chart_controller.dart'; import 'vm_chart_controller.dart'; class MemoryChartPaneController extends DisposableController with AutoDisposeControllerMixin { - MemoryChartPaneController(this.mode, {this.isDeviceAndroid}) + MemoryChartPaneController(this.mode, {bool? isDeviceAndroid}) : assert( mode == DevToolsMode.connected || isDeviceAndroid != null, 'If application is not connected, isDeviceAndroid must be provided.', - ) { + ), + data = ChartData(isDeviceAndroid: isDeviceAndroid) { unawaited(_init()); } @@ -34,13 +34,22 @@ class MemoryChartPaneController extends DisposableController ); } + Map toJson() { + // TODO(polina-c): implement, https://github.com/flutter/devtools/issues/6972 + return {}; + } + DevToolsMode mode; - /// Wether device is android, if [mode] is not [DevToolsMode.connected]. - /// - /// If [mode] is [DevToolsMode.connected], this value should be detected - /// by [_chartConnection]. - final bool? isDeviceAndroid; + final ChartData data; + + late final ChartConnection? _chartConnection = + (mode == DevToolsMode.connected) + ? ChartConnection( + data.timeline, + isAndroidChartVisible: isAndroidChartVisible, + ) + : null; Future _init() async { _updateAndroidChartVisibility(); @@ -53,36 +62,16 @@ class MemoryChartPaneController extends DisposableController ); } - Map toJson() { - // TODO(polina-c): implement, https://github.com/flutter/devtools/issues/6972 - return {}; - } - - late final ChartConnection? _chartConnection = - (mode == DevToolsMode.connected) - ? ChartConnection( - memoryTimeline, - isAndroidChartVisible: isAndroidChartVisible, - ) - : null; - - final MemoryTimeline memoryTimeline = MemoryTimeline(); - late final EventChartController event = - EventChartController(memoryTimeline, paused: paused); + EventChartController(data.timeline, paused: paused); late final VMChartController vm = - VMChartController(memoryTimeline, paused: paused); + VMChartController(data.timeline, paused: paused); late final AndroidChartController android = AndroidChartController( - memoryTimeline, + data.timeline, sharedLabels: vm.labelTimestamps, paused: paused, ); - ValueListenable get isLegendVisible => _legendVisibleNotifier; - final _legendVisibleNotifier = ValueNotifier(true); - bool toggleLegendVisibility() => - _legendVisibleNotifier.value = !_legendVisibleNotifier.value; - ValueNotifier isChartVisible = preferences.memory.showChart; void resetAll() { @@ -103,17 +92,6 @@ class MemoryChartPaneController extends DisposableController android.dirty = true; } - /// Default is to display default tick width based on width of chart of the collected - /// data in the chart. - final _displayInterval = - ValueNotifier(ChartInterval.theDefault); - - set displayInterval(ChartInterval interval) { - _displayInterval.value = interval; - } - - ChartInterval get displayInterval => _displayInterval.value; - ValueListenable get paused => _paused; final _paused = ValueNotifier(true); void pause() => _paused.value = true; @@ -127,7 +105,7 @@ class MemoryChartPaneController extends DisposableController final isAndroidChartVisible = ValueNotifier(false); void _updateAndroidChartVisibility() { - final isAndroid = isDeviceAndroid ?? _chartConnection!.isDeviceAndroid; + final isAndroid = data.isDeviceAndroid ?? _chartConnection!.isDeviceAndroid; isAndroidChartVisible.value = isAndroid && preferences.memory.androidCollectionEnabled.value; } @@ -135,8 +113,7 @@ class MemoryChartPaneController extends DisposableController @override void dispose() { super.dispose(); - _legendVisibleNotifier.dispose(); - _displayInterval.dispose(); + data.dispose(); event.dispose(); vm.dispose(); android.dispose(); diff --git a/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/chart_control_pane.dart b/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/chart_control_pane.dart index b01bf845f7c..671aaafcd54 100644 --- a/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/chart_control_pane.dart +++ b/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/chart_control_pane.dart @@ -44,7 +44,7 @@ class _ChartControlPaneState extends State void _clearTimeline() { ga.select(gac.memory, gac.clear); - widget.chart.memoryTimeline.reset(); + widget.chart.data.timeline.reset(); // Remove history of all plotted data in all charts. widget.chart.resetAll(); @@ -105,9 +105,9 @@ class _LegendButton extends StatelessWidget { @override Widget build(BuildContext context) { return ValueListenableBuilder( - valueListenable: chartController.isLegendVisible, + valueListenable: chartController.data.isLegendVisible, builder: (_, legendVisible, __) => GaDevToolsButton( - onPressed: chartController.toggleLegendVisibility, + onPressed: chartController.data.toggleLegendVisibility, gaScreen: gac.memory, gaSelection: legendVisible ? gac.MemoryEvent.hideChartLegend diff --git a/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/chart_pane.dart b/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/chart_pane.dart index 977e7dcb9cb..9c0c19a5e15 100644 --- a/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/chart_pane.dart +++ b/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/chart_pane.dart @@ -98,7 +98,7 @@ class _MemoryChartPaneState extends State } final allValues = ChartsValues( - widget.chart.memoryTimeline, + widget.chart.data.timeline, isAndroidChartVisible: widget.chart.isAndroidChartVisible, index: value.index, timestamp: value.timestamp ?? _timestamp, @@ -176,7 +176,7 @@ class _MemoryChartPaneState extends State height: defaultChartHeight, child: MemoryAndroidChart( widget.chart.android, - widget.chart.memoryTimeline, + widget.chart.data.timeline, ), ), ], @@ -185,7 +185,7 @@ class _MemoryChartPaneState extends State // The legend. MultiValueListenableBuilder( listenables: [ - widget.chart.isLegendVisible, + widget.chart.data.isLegendVisible, widget.chart.isAndroidChartVisible, ], builder: (_, values, __) { diff --git a/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/interval_dropdown.dart b/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/interval_dropdown.dart index ecf237b7b33..a61ba6d2d22 100644 --- a/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/interval_dropdown.dart +++ b/packages/devtools_app/lib/src/screens/memory/panes/chart/widgets/interval_dropdown.dart @@ -37,7 +37,7 @@ class _IntervalDropdownState extends State { return RoundedDropDownButton( isDense: true, - value: widget.chartController.displayInterval, + value: widget.chartController.data.displayInterval, onChanged: (ChartInterval? newValue) { final value = newValue!; setState(() { @@ -45,7 +45,7 @@ class _IntervalDropdownState extends State { gac.memory, '${gac.MemoryEvent.chartInterval}-${value.displayName}', ); - widget.chartController.displayInterval = value; + widget.chartController.data.displayInterval = value; final duration = value.duration; widget.chartController.event.zoomDuration = duration; diff --git a/packages/devtools_app/lib/src/screens/memory/shared/primitives/memory_timeline.dart b/packages/devtools_app/lib/src/screens/memory/shared/primitives/memory_timeline.dart index fa26c0a8cc8..7b6d8e3e77b 100644 --- a/packages/devtools_app/lib/src/screens/memory/shared/primitives/memory_timeline.dart +++ b/packages/devtools_app/lib/src/screens/memory/shared/primitives/memory_timeline.dart @@ -9,89 +9,24 @@ import 'package:vm_service/vm_service.dart'; import '../../../../shared/primitives/utils.dart'; -/// All Raw data received from the VM and offline data loaded from a memory log file. +/// All Raw data received from the VM or offline data. class MemoryTimeline { - /// This flag will be needed for offline mode implementation. - final offline = false; - - /// Return the data payload that is active. - List get data => offline ? offlineData : liveData; - - int get startingIndex => offline ? offlineStartingIndex : liveStartingIndex; - - set startingIndex(int value) { - offline ? offlineStartingIndex = value : liveStartingIndex = value; - } - int get endingIndex => data.isNotEmpty ? data.length - 1 : -1; /// Raw Heap sampling data from the VM. - final List liveData = []; - - /// Start index of liveData plotted for MPChartData/MPEngineChartData sets. - int liveStartingIndex = 0; - - /// Data of the last selected offline memory source (JSON file in /tmp). - final List offlineData = []; - - /// Start index of offlineData plotted for MPChartData/MPEngineChartData sets. - int offlineStartingIndex = 0; + final List data = []; /// Extension Events. - final _eventFiredNotifier = ValueNotifier(null); - ValueListenable get eventNotifier => _eventFiredNotifier; + final _eventFiredNotifier = ValueNotifier(null); /// Notifies that a new Heap sample has been added to the timeline. - final _sampleAddedNotifier = ValueNotifier(null); - ValueListenable get sampleAddedNotifier => _sampleAddedNotifier; + final _sampleAddedNotifier = ValueNotifier(null); /// List of events awaiting to be posted to HeapSample. final _eventSamples = []; - void postEventSample(EventSample event) { -/* - final lastEvent = _eventSamples.safeLast; - if (lastEvent != null) { - final lastTime = Duration(milliseconds: lastEvent.timestamp); - final eventTime = Duration(milliseconds: event.timestamp); - if ((lastTime + MemoryTimeline.updateDelay).compareTo(eventTime) <= 0) { - // Coalesce new event to old event. - _eventSamples.add(EventSample( -// lastEvent.timestamp, - event.timestamp, - lastEvent.isEventGC || event.isEventGC, - lastEvent.isEventSnapshot || event.isEventSnapshot, - lastEvent.isEventSnapshotAuto || event.isEventSnapshotAuto, - lastEvent.allocationAccumulator, - )); - } - } -*/ - _eventSamples.add(event); - } - - void addSnapshotEvent({bool auto = false}) { - postEventSample( - EventSample.snapshotEvent( - DateTime.now().millisecondsSinceEpoch, - snapshotAuto: auto, - events: extensionEvents, - ), - ); - } - - void addGCEvent() { - final timestamp = DateTime.now().millisecondsSinceEpoch; - postEventSample( - EventSample.gcEvent( - timestamp, - events: extensionEvents, - ), - ); - } - bool get anyEvents => peekEventTimestamp != -1; /// Peek at next event to pull, if no event return -1 timestamp. @@ -100,45 +35,34 @@ class MemoryTimeline { return event != null ? event.timestamp : -1; } - /// Grab and remove the event to be posted. - EventSample pullEventSample() { - final result = _eventSamples.first; - _eventSamples.removeAt(0); - return result; + ValueNotifier get sampleEventNotifier => _sampleEventNotifier; + final _sampleEventNotifier = ValueNotifier(0); + + ExtensionEvents? get extensionEvents { + if (_extensionEvents.isNotEmpty) { + final eventsToProcess = ExtensionEvents(_extensionEvents.toList()); + _extensionEvents.clear(); + return eventsToProcess; + } + return null; } - final _sampleEventNotifier = ValueNotifier(0); + final _extensionEvents = []; - ValueNotifier get sampleEventNotifier => _sampleEventNotifier; + bool get anyPendingExtensionEvents => _extensionEvents.isNotEmpty; /// Whether the timeline has been manually paused via the Pause button. bool manuallyPaused = false; void reset() { - liveData.clear(); - startingIndex = 0; + data.clear(); } - /// Common utility function to handle loading of the data into the - /// chart for either offline or live Feed. - static final DateFormat _milliFormat = DateFormat('HH:mm:ss.SSS'); static String fineGrainTimestampFormat(int timestamp) => _milliFormat.format(DateTime.fromMillisecondsSinceEpoch(timestamp)); - void addSample(HeapSample sample) { - // Always record the heap sample in the raw set of data (liveFeed). - liveData.add(sample); - - // Only notify that new sample has arrived if the - // memory source is 'Live Feed'. - if (!offline) { - _sampleAddedNotifier.value = sample; - sampleEventNotifier.value++; - } - } - static const customDevToolsEvent = 'DevTools.Event'; static const devToolsExtensionEvent = '${customDevToolsEvent}_'; @@ -150,17 +74,10 @@ class MemoryTimeline { MemoryTimeline.devToolsExtensionEvent.length, ); - final _extensionEvents = []; - - bool get anyPendingExtensionEvents => _extensionEvents.isNotEmpty; - - ExtensionEvents? get extensionEvents { - if (_extensionEvents.isNotEmpty) { - final eventsToProcess = ExtensionEvents(_extensionEvents.toList()); - _extensionEvents.clear(); - return eventsToProcess; - } - return null; + void addSample(HeapSample sample) { + data.add(sample); + _sampleAddedNotifier.value = sample; + sampleEventNotifier.value++; } void addExtensionEvent( @@ -175,4 +92,31 @@ class MemoryTimeline { _extensionEvents.add(extensionEvent); } + + /// Grab and remove the event to be posted. + EventSample pullEventSample() { + final result = _eventSamples.first; + _eventSamples.removeAt(0); + return result; + } + + void addSnapshotEvent({bool auto = false}) { + _eventSamples.add( + EventSample.snapshotEvent( + DateTime.now().millisecondsSinceEpoch, + snapshotAuto: auto, + events: extensionEvents, + ), + ); + } + + void addGCEvent() { + final timestamp = DateTime.now().millisecondsSinceEpoch; + _eventSamples.add( + EventSample.gcEvent( + timestamp, + events: extensionEvents, + ), + ); + } } diff --git a/packages/devtools_app/test/shared/json_to_service_cache_test.dart b/packages/devtools_app/test/shared/json_to_service_cache_test.dart index cda7e53a62d..ded5554ca75 100644 --- a/packages/devtools_app/test/shared/json_to_service_cache_test.dart +++ b/packages/devtools_app/test/shared/json_to_service_cache_test.dart @@ -137,41 +137,43 @@ void main() { }); }); - test('instanceToJson converts Instances back to the JSON that created them', - () { - const data = { - 'id': 1, - 'map': { - 'foo': 'bar', - 'baz': [ - 'a', + test( + 'instanceToJson converts Instances back to the JSON that created them', + () { + const data = { + 'id': 1, + 'map': { + 'foo': 'bar', + 'baz': [ + 'a', + null, + ], + }, + 'list': [ + [ + 7, + '8', + 9.0, + ], + 1, + '2', + 4.9, + true, null, + { + 'a': 'b', + 'c': 'd', + }, ], - }, - 'list': [ - [ - 7, - '8', - 9.0, - ], - 1, - '2', - 4.9, - true, - null, - { - 'a': 'b', - 'c': 'd', - }, - ], - 'aNullValue': null, - }; + 'aNullValue': null, + }; - final cache = JsonToServiceCache(); - final root = cache.insertJsonObject(data); + final cache = JsonToServiceCache(); + final root = cache.insertJsonObject(data); - final rootConvertedBackToJson = cache.instanceToJson(root); + final rootConvertedBackToJson = cache.instanceToJson(root); - expect(rootConvertedBackToJson, data); - }); + expect(rootConvertedBackToJson, data); + }, + ); } diff --git a/packages/devtools_app/test/test_infra/scenes/memory/default.dart b/packages/devtools_app/test/test_infra/scenes/memory/default.dart index 1d483cbf86b..9d3b6c34404 100644 --- a/packages/devtools_app/test/test_infra/scenes/memory/default.dart +++ b/packages/devtools_app/test/test_infra/scenes/memory/default.dart @@ -153,8 +153,8 @@ class MemoryDefaultScene extends Scene { connectedDiff: diffController, connectedProfile: profileController, ) - ..chart.memoryTimeline.offlineData.clear() - ..chart.memoryTimeline.offlineData.addAll(memoryJson.data); + ..chart.data.timeline.data.clear() + ..chart.data.timeline.data.addAll(memoryJson.data); } @override