search

firefox-devtools / profiler

Firefox Profiler — Web app for Firefox performance analysis
https://profiler.firefox.com
Mozilla Public License 2.0
1.14k stars 372 forks source link

Do we rely on the order of markers anywhere? #5021

Open mstange opened 3 weeks ago

mstange commented 3 weeks ago

Do we rely on the order of markers anywhere? The answer to this question might just be "no", and that would actually make me very happy. But I'm a little hesitant to just assume that the ordering is irrelevant.

Background:

The marker table has a startTime and an endTime column. There are no comments on the order of timestamps in these columns: https://github.com/firefox-devtools/profiler/blob/af62509321b9610b7acacb7e4094eccd99945a35/src/types/profile.js#L257-L258

A long time ago, the marker table only had a time column, and, starting with version 1, that column was guaranteed to be sorted: https://github.com/firefox-devtools/profiler/blob/af62509321b9610b7acacb7e4094eccd99945a35/src/profile-logic/processed-profile-versioning.js#L256-L261

But in version 30 / 31, we changed the marker table to have startTime and endTime columns: https://github.com/firefox-devtools/profiler/blob/af62509321b9610b7acacb7e4094eccd99945a35/src/profile-logic/processed-profile-versioning.js#L1354-L1355

There are no comments about any ordering requirements on these new columns. And maybe there are no such requirements!

I'm just filing this issue so that we can audit the code and be sure, and maybe to fix some comments.

I found one comment that says "markers are ordered by start time" in the tooltip test, but it might be outdated: https://github.com/firefox-devtools/profiler/blob/af62509321b9610b7acacb7e4094eccd99945a35/src/test/components/TooltipMarker.test.js#L528-L529

I checked the implementation of the "drop samples outside markers" transform, and it doesn't rely on an order; this call to canonicalizeRangeSet does the ordering: https://github.com/firefox-devtools/profiler/blob/af62509321b9610b7acacb7e4094eccd99945a35/src/profile-logic/transforms.js#L1693

julienw commented 2 weeks ago

They're sorted in https://github.com/firefox-devtools/profiler/blob/af62509321b9610b7acacb7e4094eccd99945a35/src/selectors/per-thread/markers.js#L132-L134

I'm pretty sure we don't rely on them being sorted in the pieces of code that deal with them in the raw tables. However the marker list retrieved from the selectors is sorted, so the code comments are not wrong :-)