catapult-project / catapult

Deprecated Catapult GitHub. Please instead use http://crbug.com "Speed>Benchmarks" component for bugs and https://chromium.googlesource.com/catapult for downloading and editing source code..
https://chromium.googlesource.com/catapult
BSD 3-Clause "New" or "Revised" License
1.91k stars 563 forks source link

Tie break slices by length as well #4491

Open bkin opened 6 years ago

bkin commented 6 years ago

Consider this json:

[
  { "ph":"X", "pid":"good", "ts":0, "dur":100, "name": "parent" },
  { "ph":"X", "pid":"good", "ts":0, "dur":90, "name": "child" },
  { "ph":"X", "pid":"also_good", "ts":1, "dur":90, "name": "child" },
  { "ph":"X", "pid":"also_good", "ts":0, "dur":100, "name": "parent" },
  { "ph":"X", "pid":"bad", "ts":0, "dur":90, "name": "child" },
  { "ph":"X", "pid":"bad", "ts":0, "dur":100, "name": "parent" },

The pid "bad" looks wrong, since parent and child overlap. trace2html sorts slices by start time, which is why pid "also_good" works, but the tie breaks implemented do not return the longer slice first.

The following patch fixes the issue for me for complete slices:

diff --git a/tracing/tracing/model/slice_group.html b/tracing/tracing/model/slice_group.html
index f8e64a5..5dc5147 100644
--- a/tracing/tracing/model/slice_group.html
+++ b/tracing/tracing/model/slice_group.html
@@ -464,6 +464,11 @@ tr.exportTo('tr.model', function() {
           return x.start - y.start;
         }

+        // First tie-break: Longer slice first
+        if (x.end !== y.end) {
+          return y.end - x.end;
+        }
+
         // Elements get inserted into the slices array in order of when the
         // slices start. Because slices must be properly nested, we break
         // start-time ties by assuming that the elements appearing earlier

There are more instances where slices are sorted. I am not sure if I can figure out all that would benefit from this additional tie-break