Integrate Ensogl stats with profiling framework #3388

MichaelMauderer · 2022-04-08T12:15:14Z

Pull Request Description

Add logging of EnsoGL performance stats to the profiling framework. Also extends the visualization in the debug scene to show an overview of the performance stats. We now render a timeline of blocks that indicate by their colour the rough FPS range we are in:

Peek.2022-04-08.14-08.mp4

Important Notes

[ci no changelog needed]

Needs to be merged after #3382 as it requires some changes about metadata logging from there. That is why this PR is currently still in draft mode and based on that branch.

Checklist

Please include the following checklist in your PR:

The documentation has been updated if necessary.
All code conforms to the Scala, Java, and Rust style guides.
All code has been tested:
- Unit tests have been written where possible.
- If GUI codebase was changed: Enso GUI was tested when built using BOTH ./run dist and ./run watch.

kazcw · 2022-04-19T16:52:13Z

lib/rust/ensogl/component/flame-graph/src/lib.rs

+
+fn align_mark(mut mark: profiler_flame_graph::Mark, origin_x: f64) -> profiler_flame_graph::Mark {
+    mark.position -= origin_x;
+    mark.position += X_SCALE;


kazcw · 2022-04-19T16:58:09Z

lib/rust/ensogl/core/src/display/world.rs

+
+
+thread_local! {
+    /// A common preamble used to start every shader program.


Comment from somewhere else?

kazcw · 2022-04-19T17:01:49Z

lib/rust/ensogl/component/flame-graph/src/lib.rs

@@ -146,6 +149,29 @@ impl FlameGraph {
    pub fn marks(&self) -> &[Mark] {
        &self.marks
    }
+
+    /// Add additional blocks to the visualisation.
+    pub fn add_blocks<Blocks: Iterator<Item = profiler_flame_graph::Block>>(


API would be simpler with a add_block interface here; caller can handle the looping

kazcw · 2022-04-19T17:01:56Z

lib/rust/ensogl/component/flame-graph/src/lib.rs

+    }
+
+    /// Add additional marks to the visualisation.
+    pub fn add_marks<Marks: Iterator<Item = profiler_flame_graph::Mark>>(&mut self, marks: Marks) {


Same as above

kazcw · 2022-04-19T17:11:14Z

lib/rust/ensogl/example/profiling-run-graph/src/lib.rs

+        .iter_metadata()
+        .filter(|metadata| matches!(metadata.data, Metadata::RenderStats(_)));
+    for (prev, current) in render_stats.tuple_windows() {
+        if let Metadata::RenderStats(data) = current.data {


The filter/if let seem redundant here; why not use a filter_map operation to get a correctly-type iterator?

Maybe I'm missing something, but using filter_map, I end up with something like

profile .iter_metadata() .filter(|metadata| matches!(metadata.data, Metadata::RenderStats(_))) .tuple_windows() .filter_map(|(prev, current)| { if let Metadata::RenderStats(data) = current.data { Some((prev, current, data)) } else { None } }) .for_each(|(prev, current, data)| { let start = prev.mark.into_ms(); let end = current.mark.into_ms(); let row = -1; let label = format!("{:#?}", data); let theme_color = match data.fps { fps if fps > 55.0 => COLOR_FPS_GOOD, fps if fps > 25.0 => COLOR_FPS_MEDIUM, _ => COLOR_FPS_BAD, }; let block = profiler_flame_graph::Block { start, end, row, label, theme_color }; blocks.push(block); });

This doesn't seem really any better. Instead, we have a three tuple as an additional step. I don't see a good other way to retrieve the content of the Metadata Enum in some more elegant way, and we do need both previous, current and the data to generate the block.

Could do this:

let render_stats = profile .iter_metadata() .filter_map(|metadata| match metadata.data { Metadata::RenderStats(data) => { let mark = metadata.mark; Some(enso_profiler_data::Metadata { mark, data }) }, _ => None, }); for (prev, current) in render_stats.tuple_windows() { let start = prev.mark.into_ms(); let end = current.mark.into_ms(); let row = -1; let label = format!("{:#?}", current.data); let block_type = match current.data.fps { fps if fps > 55.0 => Performance::Good, fps if fps > 25.0 => Performance::Medium, _ => Performance::Bad, }; let block = profiler_flame_graph::Block { start, end, row, label, block_type }; blocks.push(block); }

I have named enso_profiler_data::Metadata and its fields poorly; in a future PR I think I will rename it to Timestamped with a time field, so that usages like this would be more transparent.

Neat! Didn't think of re-using the Metadata with the inner data type. I'll do that its clearer.

lib/rust/ensogl/example/profiling-run-graph/src/lib.rs

kazcw · 2022-04-19T17:25:12Z

lib/rust/profiler/flame-graph/src/lib.rs

-    pub state: State,
+    pub label:       String,
+    /// Indicates the name of the color for lookup in the theme.
+    pub theme_color: &'static str,


i liked the abstraction State provided here--profiler_flame_graph::Block was a high-level description of an element of a graph, and shape_from_block lowered it to the ensogl flame_graph::Block, which was where the rendering details came in.

I see that a simple enum like State no longer works now that different types of graph need to classify Blocks differently. I think although it would be a little more complex than this lowering-early approach, it might be worth introducing a parameterization like profiler_flame_graph::Block<BlockType> (where ensogl's flame_graph contains something like impl From<SomeBlockType> for ThemeColor for each concrete BlockType) in order to maintain the clear boundary between the responsibilities of the two crates and their Block types.

This would require fixing a bit of a hack from my original implementation of hybrid graph: There shouldn't be a hybrid graph type containing blocks arranged according to different disciplines; it would be better for the render-profile scene to draw a callgraph, draw a rungraph under it, and draw the new 1-bar-high frametime graph in the stack too. I just did it the current way because it was easier due to my inexperience with Enso's FRP networks.

Excellent explanation @kazcw! Thanks for writing it.

This is an excellent approach! I made the changes. There are a few tweaks that deviate from your proposal, for example, not using From due to orphan rule issues, but a new trait, which seemed nicer than a newtype to make the conversion work. But have a look and let me know if you think that can still be improved.

This would require fixing a bit of a hack from my original implementation of hybrid graph: There shouldn't be a hybrid graph type containing blocks arranged according to different disciplines; it would be better for the render-profile scene to draw a callgraph, draw a rungraph under it, and draw the new 1-bar-high frametime graph in the stack too. I just did it the current way because it was easier due to my inexperience with Enso's FRP networks.

Adding Sections like this is on my agenda for the visualization, but I think here it is not yet needed. This change will probably come for the multiprocess visualization.

wdanilo · 2022-04-20T06:07:49Z

lib/rust/ensogl/example/profiling-run-graph/src/lib.rs

+            let label = format!("{:#?}", data);
+            let theme_color = match data.fps {
+                fps if fps > 55.0 => COLOR_FPS_GOOD,
+                fps if fps > 25.0 => COLOR_FPS_MEDIUM,


I don't think "medium" is the right word between "good" and "bad". I cannot think of a good word now, but "medium" sounds off too me. To be confirmed with a native speaker.

I changed the terminology a bit and instead of “FPS” I am using Performance. For that, “Medium” seems to fit better.

wdanilo · 2022-04-20T06:08:23Z

lib/rust/profiler/flame-graph/src/lib.rs

-    pub state: State,
+    pub label:       String,
+    /// Indicates the name of the color for lookup in the theme.
+    pub theme_color: &'static str,


Excellent explanation @kazcw! Thanks for writing it.

kazcw · 2022-04-20T15:08:49Z

lib/rust/ensogl/example/profiling-run-graph/src/lib.rs

+        .iter_metadata()
+        .filter(|metadata| matches!(metadata.data, Metadata::RenderStats(_)));
+    for (prev, current) in render_stats.tuple_windows() {
+        if let Metadata::RenderStats(data) = current.data {


Could do this:

let render_stats = profile .iter_metadata() .filter_map(|metadata| match metadata.data { Metadata::RenderStats(data) => { let mark = metadata.mark; Some(enso_profiler_data::Metadata { mark, data }) }, _ => None, }); for (prev, current) in render_stats.tuple_windows() { let start = prev.mark.into_ms(); let end = current.mark.into_ms(); let row = -1; let label = format!("{:#?}", current.data); let block_type = match current.data.fps { fps if fps > 55.0 => Performance::Good, fps if fps > 25.0 => Performance::Medium, _ => Performance::Bad, }; let block = profiler_flame_graph::Block { start, end, row, label, block_type }; blocks.push(block); }

I have named enso_profiler_data::Metadata and its fields poorly; in a future PR I think I will rename it to Timestamped with a time field, so that usages like this would be more transparent.

MichaelMauderer self-assigned this Apr 8, 2022

MichaelMauderer requested a review from kazcw April 8, 2022 12:15

Base automatically changed from wip/michaelmauderer/backend_communication_profiling to develop April 19, 2022 11:30

MichaelMauderer added 4 commits April 19, 2022 13:35

Add ability to render marks in visualisation.

bfada7d

Expand profiling. Update visualisation to show rpc events.

09171ca

Integrate EnsoGL stats with profiling framework and visualisation.

acee3c3

Rebase and update.

a78eb90

MichaelMauderer force-pushed the wip/michaelmauderer/ensogl_stats_181151201 branch from 334456a to a78eb90 Compare April 19, 2022 12:40

Increase wasm size limit.

ff9782f

MichaelMauderer marked this pull request as ready for review April 19, 2022 14:09

MichaelMauderer requested review from wdanilo, farmaazon, mwu-tow and 4e6 as code owners April 19, 2022 14:09

kazcw reviewed Apr 19, 2022

View reviewed changes

wdanilo approved these changes Apr 20, 2022

View reviewed changes

MichaelMauderer and others added 2 commits April 20, 2022 12:24

Review feedback.

a3de180

Merge branch 'develop' into wip/michaelmauderer/ensogl_stats_181151201

3f65bce

MichaelMauderer requested a review from kazcw April 20, 2022 10:24

Fix tests.

2940f6e

kazcw approved these changes Apr 20, 2022

View reviewed changes

MichaelMauderer added 2 commits April 21, 2022 10:20

Refactor unwrapping/iteration.

08835e9

Merge branch 'develop' into wip/michaelmauderer/ensogl_stats_181151201

4ed3f47

MichaelMauderer added the CI: Ready to merge This PR is eligible for automatic merge label Apr 21, 2022

mergify bot merged commit e8342b0 into develop Apr 21, 2022

mergify bot deleted the wip/michaelmauderer/ensogl_stats_181151201 branch April 21, 2022 09:38

wdanilo mentioned this pull request Feb 6, 2023

Developers should have a visualization that can show information from the backend-profiling. #4311

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate Ensogl stats with profiling framework #3388

Integrate Ensogl stats with profiling framework #3388

MichaelMauderer commented Apr 8, 2022

kazcw Apr 19, 2022

kazcw Apr 19, 2022

kazcw Apr 19, 2022

kazcw Apr 19, 2022

kazcw Apr 19, 2022

MichaelMauderer Apr 20, 2022

kazcw Apr 20, 2022

MichaelMauderer Apr 21, 2022

kazcw Apr 19, 2022

wdanilo Apr 20, 2022

MichaelMauderer Apr 20, 2022

wdanilo Apr 20, 2022

MichaelMauderer Apr 20, 2022

wdanilo Apr 20, 2022

kazcw Apr 20, 2022



		thread_local! {
		/// A common preamble used to start every shader program.

Integrate Ensogl stats with profiling framework #3388

Integrate Ensogl stats with profiling framework #3388

Conversation

MichaelMauderer commented Apr 8, 2022

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment