Don't merge scopes that have different data (#102)

Previously we would merge all scopes that have the same `id`, but if you
have multiple scopes after each that have same `id` but different
dynamic `data` field they would be merged together and we wouldn't
display the `data` field.

This is something we ran into in our codebase that made it harder to see
why a certain heavy operation was happening as we were missing the
context for it. Which we did specify but was merged together and not
shown here as merging scopes is on by default (and is a general good
idea).

Adds a simple visual test for in the imgui and egui clients.

Resolves: #73

## Example

This is how it looks like now:


![image](https://user-images.githubusercontent.com/1262692/196470164-64076531-647c-48dc-84d3-7bb1a3d502a6.png)

from the following code:
```rust
// test to verify these spikes timers are not merged together as they have different data
for (name, ms) in [("First".to_string(), 20), ("Second".to_string(), 15)] {
    puffin::profile_scope!("Spike", name);
    std::thread::sleep(std::time::Duration::from_millis(ms))
}
// these are however fine to merge together as data is the same
for (_name, ms) in [("First".to_string(), 20), ("Second".to_string(), 15)] {
    puffin::profile_scope!("Spike");
    std::thread::sleep(std::time::Duration::from_millis(ms))
}
```

Before this PR this would have been a single "4x Spike"

CHANGELOG.md

@@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 <!-- next-header -->
 ## [Unreleased] - ReleaseDate
+
+### Fixed
+* [PR#102](https://github.com/EmbarkStudios/puffin/pull/102) Add a runtime setting on frameview to pack or not
+
 ## [0.13.3] - 2022-05-04
 ### Changed
 * [PR#83](https://github.com/EmbarkStudios/puffin/pull/83) Add a runtime setting on frameview to pack or not

puffin-imgui/examples/imgui.rs

@@ -152,6 +152,18 @@ fn main() {
             puffin::profile_scope!("Big spike");
             std::thread::sleep(std::time::Duration::from_millis(50))
         }
+        if frame_counter % 55 == 0 {
+            // test to verify these spikes timers are not merged together as they have different data
+            for (name, ms) in [("First".to_string(), 20), ("Second".to_string(), 15)] {
+                puffin::profile_scope!("Spike", name);
+                std::thread::sleep(std::time::Duration::from_millis(ms))
+            }
+            // these are however fine to merge together as data is the same
+            for (_name, ms) in [("First".to_string(), 20), ("Second".to_string(), 15)] {
+                puffin::profile_scope!("Spike");
+                std::thread::sleep(std::time::Duration::from_millis(ms))
+            }
+        }
 
         for _ in 0..1000 {
             puffin::profile_scope!("very thin");

puffin/src/merge.rs

@@ -1,15 +1,22 @@
 use crate::{NanoSecond, Reader, Result, Scope, Stream, ThreadInfo, UnpackedFrameData};
 use std::collections::BTreeMap;
 
+/// Temporary structure while building a [`MergeScope`].
+#[derive(Clone, Debug, PartialEq, PartialOrd, Eq, Ord, Hash)]
+struct MergeId<'s> {
+    id: &'s str,
+    data: &'s str,
+}
+
 /// Temporary structure while building a [`MergeScope`].
 #[derive(Default)]
 struct MergeNode<'s> {
     /// These are the raw scopes that got merged into us.
     /// All these scopes have the same `id`.
     pieces: Vec<MergePiece<'s>>,
 
-    /// indexed by their id
-    children: BTreeMap<&'s str, MergeNode<'s>>,
+    /// indexed by their id+data
+    children: BTreeMap<MergeId<'s>, MergeNode<'s>>,
 }
 
 #[derive(Clone, Copy, Debug, PartialEq)]
@@ -65,13 +72,19 @@ impl<'s> MergeNode<'s> {
 
         for child in Reader::with_offset(stream, piece.scope.child_begin_position)? {
             let child = child?;
-            self.children.entry(child.record.id).or_default().add(
-                stream,
-                MergePiece {
-                    relative_start_ns: child.record.start_ns - piece.scope.record.start_ns,
-                    scope: child,
-                },
-            )?;
+            self.children
+                .entry(MergeId {
+                    id: child.record.id,
+                    data: child.record.data,
+                })
+                .or_default()
+                .add(
+                    stream,
+                    MergePiece {
+                        relative_start_ns: child.record.start_ns - piece.scope.record.start_ns,
+                        scope: child,
+                    },
+                )?;
         }
 
         Ok(())
@@ -116,7 +129,7 @@ impl<'s> MergeNode<'s> {
     }
 }
 
-fn build<'s>(nodes: BTreeMap<&'s str, MergeNode<'s>>, num_frames: i64) -> Vec<MergeScope<'s>> {
+fn build<'s>(nodes: BTreeMap<MergeId<'s>, MergeNode<'s>>, num_frames: i64) -> Vec<MergeScope<'s>> {
     let mut scopes: Vec<_> = nodes
         .into_values()
         .map(|node| node.build(num_frames))
@@ -135,26 +148,32 @@ fn build<'s>(nodes: BTreeMap<&'s str, MergeNode<'s>>, num_frames: i64) -> Vec<Me
     scopes
 }
 
-/// For the given thread, merge all scopes with the same id path.
+/// For the given thread, merge all scopes with the same id+data path.
 pub fn merge_scopes_for_thread<'s>(
     frames: &'s [std::sync::Arc<UnpackedFrameData>],
     thread_info: &ThreadInfo,
 ) -> Result<Vec<MergeScope<'s>>> {
-    let mut top_nodes: BTreeMap<&'s str, MergeNode<'s>> = Default::default();
+    let mut top_nodes: BTreeMap<MergeId<'s>, MergeNode<'s>> = Default::default();
 
     for frame in frames {
         if let Some(stream_info) = frame.thread_streams.get(thread_info) {
             let offset_ns = frame.meta.range_ns.0 - frames[0].meta.range_ns.0; // make everything relative to first frame
 
             let top_scopes = Reader::from_start(&stream_info.stream).read_top_scopes()?;
             for scope in top_scopes {
-                top_nodes.entry(scope.record.id).or_default().add(
-                    &stream_info.stream,
-                    MergePiece {
-                        relative_start_ns: scope.record.start_ns - offset_ns,
-                        scope,
-                    },
-                )?;
+                top_nodes
+                    .entry(MergeId {
+                        id: scope.record.id,
+                        data: scope.record.data,
+                    })
+                    .or_default()
+                    .add(
+                        &stream_info.stream,
+                        MergePiece {
+                            relative_start_ns: scope.record.start_ns - offset_ns,
+                            scope,
+                        },
+                    )?;
             }
         }
     }

puffin_egui/examples/eframe.rs

@@ -41,6 +41,18 @@ impl eframe::App for ExampleApp {
             puffin::profile_scope!("Big spike");
             std::thread::sleep(std::time::Duration::from_millis(50))
         }
+        if self.frame_counter % 55 == 0 {
+            // test to verify these spikes timers are not merged together as they have different data
+            for (name, ms) in [("First".to_string(), 20), ("Second".to_string(), 15)] {
+                puffin::profile_scope!("Spike", name);
+                std::thread::sleep(std::time::Duration::from_millis(ms))
+            }
+            // these are however fine to merge together as data is the same
+            for (_name, ms) in [("First".to_string(), 20), ("Second".to_string(), 15)] {
+                puffin::profile_scope!("Spike");
+                std::thread::sleep(std::time::Duration::from_millis(ms))
+            }
+        }
 
         for _ in 0..1000 {
             puffin::profile_scope!("very thin");