-
Notifications
You must be signed in to change notification settings - Fork 329
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.
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
Initial frame based timeline flame chart. #1336
Conversation
// TODO(kenz): is creating all the FlameChartNode objects expensive even if | ||
// we won't add them to the view? We create all the FlameChartNode objects | ||
// and place them in FlameChart rows, but we only add [nodesInViewport] to | ||
// the widget tree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DaveShuckerow thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating the FlameChartNode objects is cheap as they are just simple stateless widgets so it is no different than creating any other single Dart object. Creating them on demand would be a bit better but isn't crucial.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to want to lazily build both horizontally and vertically. We don't want to build any widgets that we don't want to show.
ListView.builder is capable of this.
We can build one enclosing vertical view, then for each row one horizontal list view, with linked scroll controllers so that they scroll together.
https://api.flutter.dev/flutter/widgets/TrackingScrollController-class.html May be able to help with this.
ListView.builder(
controller: normalVerticalController,
itemBuilder: (context, row) {
return ListView.builder(
scrollDirection: Axis.horizontal,
controller: _trackingHorizontalController,
itemBuilder: (context, column) => buildCell(context, row, column)
);
},
);
This may take a bit of playing with to get it building the widgets that we place at different horizontal positions.
As an intermediate step, if you can know which rows to build already (since they're all the same height), you can use the ListView.builder to build just rows, then create a Stack for each row, containing all the items in the row.
ListView.builder(
itemBuilder: (context, row) {
return SingleChildScrollView(
child: Stack(children: [every positioned child from the row that fits the current viewport]),
);
},
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets sync on this tomorrow. There are some details that make a List of lists not a good fit for this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed this offline. We will leave the code as is for now. Once we have a better "worst-case" scenario to play with (full timeline flame chart or CPU profile flame chart), we will see what the performance looks like.
At that point, we can
A) start by making this view a ListView of Stacks to get the lazy benefit of a ListView on the Y scrolling axis, and if that still doesn't solve performance issues we can
B) attempt to optimize in the horizontal direction with slivers and/or custom layout logic
void didChangeDependencies() { | ||
super.didChangeDependencies(); | ||
_controller = Controllers.of(context).timeline; | ||
_selectedEventSubscription?.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use the mixin that handles managing subscriptions rather than manually cancelling.
packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart
Show resolved
Hide resolved
packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart
Show resolved
Hide resolved
packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some thoughts on how we can build widgets lazily in this environment.
packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart
Outdated
Show resolved
Hide resolved
packages/devtools_app/lib/src/timeline/flutter/timeline_flame_chart.dart
Outdated
Show resolved
Hide resolved
// TODO(kenz): is creating all the FlameChartNode objects expensive even if | ||
// we won't add them to the view? We create all the FlameChartNode objects | ||
// and place them in FlameChart rows, but we only add [nodesInViewport] to | ||
// the widget tree. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're going to want to lazily build both horizontally and vertically. We don't want to build any widgets that we don't want to show.
ListView.builder is capable of this.
We can build one enclosing vertical view, then for each row one horizontal list view, with linked scroll controllers so that they scroll together.
https://api.flutter.dev/flutter/widgets/TrackingScrollController-class.html May be able to help with this.
ListView.builder(
controller: normalVerticalController,
itemBuilder: (context, row) {
return ListView.builder(
scrollDirection: Axis.horizontal,
controller: _trackingHorizontalController,
itemBuilder: (context, column) => buildCell(context, row, column)
);
},
);
This may take a bit of playing with to get it building the widgets that we place at different horizontal positions.
As an intermediate step, if you can know which rows to build already (since they're all the same height), you can use the ListView.builder to build just rows, then create a Stack for each row, containing all the items in the row.
ListView.builder(
itemBuilder: (context, row) {
return SingleChildScrollView(
child: Stack(children: [every positioned child from the row that fits the current viewport]),
);
},
);
); | ||
rows[gpuSectionStartRow].nodes.add(gpuSectionLabel); | ||
|
||
void createChartNodes(TimelineEvent event, int row) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to pass in the bounds of the viewport here and use this to determine which events from the row to create a node for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will leave this as is for now, as discussed offline. I'll add a TODO to track the suggestion for when we try to optimize this code later.
Lets sync up tomorrow morning on how to make them lazy. I think we can reuse the techniques from ListView rather than nesting list views. The next step if you need to support the lines drawn between associated nodes and that will not work well with nested list views but will work well if we borrow the ideas from the nested list view case to lazily only add slivers that are in view. |
.toDouble(); | ||
} | ||
|
||
// Pixels per microsecond in order to fit the entire frame in view. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that in Flutter, layouts are computed in dp.
More about this in devicePixelRatio
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be an issue as long as widget.startingContentWidth
(which is passed in with the value constraints.maxWidth
) is in the same dp as the width/height values we give each flame chart node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dave I think you had a typo. Flutter layouts are computed in logical pixels not device pixels which is a very good thing.
If they were in device pixels we would have to do a ton of normalizing everywhere to convert back to the appropriate # of device pixels to keep the ui at consistent sizes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dave I think you had a typo. Flutter layouts are computed in logical pixels not device pixels which is a very good thing.
If they were in device pixels we would have to do a ton of normalizing everywhere to convert back to the appropriate # of device pixels to keep the ui at consistent sizes.
The abbreviation for logical pixels is dp (in contrast with px). The term comes from Android.
https://stackoverflow.com/questions/2025282/what-is-the-difference-between-px-dip-dp-and-sp
I don't think this should be an issue as long as widget.startingContentWidth (which is passed in with the value constraints.maxWidth) is in the same dp as the width/height values we give each flame chart node.
Correct, just pointing out that the measurements we do are dp and not px.
@@ -38,12 +38,13 @@ class TimelineScreenBody extends StatefulWidget { | |||
} | |||
|
|||
class TimelineScreenBodyState extends State<TimelineScreenBody> { | |||
@visibleForTesting | |||
final controller = TimelineController(); | |||
TimelineController controller; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This controller is only used in build, so you don't need to refer to it from the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is, this can become a stateless widget, with the build method starting with
build(BuildContext context) {
final controller = Controllers.of(context).timeline;
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terry is working on a change that will be using it to subscribe to a listener, so I'll leave it as is in favor of avoiding merge conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is fine. Go ahead and add a TODO to document that we will be removing the stateful widget in the near future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be a stateful widget as is because the toggle switches between timeline modes, so we will not be removing the stateful widget. Terry is working on hooking the bar chart up to live data, for which he will be adding a listener subscription in didChangeDependencies. This is why I was saying I would just leave it because he is working on this right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm missing where the setState call gets triggered on mode change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not part of this PR - was landed in the initial timeline flutter PR.
void _onTimelineModeChanged(bool frameBased) {
setState(() {
controller.timelineMode =
frameBased ? TimelineMode.frameBased : TimelineMode.full;
});
}
# Conflicts: # packages/devtools_app/lib/src/timeline/flutter/timeline_screen.dart
void didChangeDependencies() { | ||
super.didChangeDependencies(); | ||
_controller = Controllers.of(context).timeline; | ||
autoDispose(_controller.onSelectedTimelineEvent.listen((_) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to call cancel() here.
Otherwise, every time the dependencies are changed, the old subscriptions will stick around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. Landing this. Will fix in the next PR I'm about to send out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Initial implementation of Timeline flame chart. Supports event selection and tooltips.
NOTE: will work on adding tests while this PR is under initial review.
TODOs