StanfordLegion / prof-viewer

Legion Prof Viewer
Apache License 2.0
0 stars 5 forks source link

Crash on initial pan left after start #39

Closed bryevdv closed 11 months ago

bryevdv commented 11 months ago

If you start the profiler and hit the left arrow then the profiler crashes immediately. This is due to an overflow in compute_sample_utilization that was masked by:

       let start_time = interval.start.0 as u64;

which is not true when panning left from the initial starting range:

assertion failed: interval.start.0 >= 0:

So, one way to deal with this is to be more careful in compute_sample_utilization:

diff --git a/tools/legion_prof_rs/src/backend/data_source.rs b/tools/legion_prof_rs/src/backend/data_source.rs
index 12a6c5392..6d8036f2e 100644
--- a/tools/legion_prof_rs/src/backend/data_source.rs
+++ b/tools/legion_prof_rs/src/backend/data_source.rs
@@ -561,7 +561,16 @@ impl StateDataSource {
         interval: ts::Interval,
         samples: u64,
     ) -> Vec<UtilPoint> {
-        let start_time = interval.start.0 as u64;
+        if interval.start.0 + interval.duration_ns() <= 0 {
+            return [].to_vec();
+        }
+
+        let start_time = if interval.start.0 < 0 {
+            0
+        } else {
+            interval.start.0 as u64
+        };
+
         let duration = interval.duration_ns() as u64;

         let first_index = step_utilization

This seems to fix the issue, panning left before the initial start-up timestamp works as expected.

Questions for @elliottslaughter:

elliottslaughter commented 11 months ago

No, I do not think we should prohibit the GUI from panning left from the origin.

I do think the GUI could be smarter about selecting tiles. That is, on this code path, intersect the request interval with the profile's actual interval:

https://github.com/StanfordLegion/prof-viewer/blob/99e0642d80c9b5af71dabb98d702fdbbdc095661/src/app.rs#L1331-L1333

At that point, it becomes invalid for the GUI to request bounds outside of the profile, and the backend can assert (if it wishes to do so) or ignore it. It's a matter of preference how defensive we want to be in this scenario.

bryevdv commented 11 months ago

Sure that also seems like a good option for the GUI https://github.com/StanfordLegion/prof-viewer/pull/40