Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove getMarkerSchemaName special cases - look up marker schemas from data.type and nothing else #5293

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

mstange
Copy link
Contributor

@mstange mstange commented Jan 6, 2025

Fixes #5275.

This removes getMarkerSchemaName and simplifies the lookup of the marker schema - the marker schema is now purely based on data.type. This avoids accidentally matching a marker to the wrong schema.

I took care to keep any desirable effects from the old handling, with an upgrader.

Tests pass after every commit.

The behavior for the following profiles remains the same:

The behavior for the following profiles is changed, to good effect:

The behavior for the following profiles is changed, and it's somewhat unfortunate:

If we want RefreshDriverTick markers to be visible in the timeline-overview area, we should fix them on the Gecko side. They've been missing in all post-schema profiles, and nobody noticed. So I think this regression for old profiles can be accepted.

@mstange mstange requested a review from canova January 6, 2025 14:40
@mstange mstange self-assigned this Jan 6, 2025
This variable is always non-null.
We likely wanted to check markerSchemaName but that's also always non-null at the moment.
…looking up the schema.

The usefulness of this special case appears to have been limited to
writing tests: Our tests contained many markers which only match a
schema due to their name.

So this patch moves the special case to the handling of TestDefinedMarkers:
When the payload is absent, we synthesize a { type: <markerName> } payload
for convenience.

Markers from Firefox never needed this special case. And the special
handling caused some trouble when markers were mistakenly matched
against a schema with a custom label that was referring to fields
which weren't present on the marker (because the marker doesn't have
a payload).
I found two cases of this in the wild, with "CPUSpeed" and "Awake"
markers:

The first CPUSpeed marker on https://share.firefox.dev/4gmLaif
has a null data field.
Before this patch, it matched a CPUSpeed schema. This gave it a
broken tableLabel ("CPUSpeed Speed = GHz").

For Awake markers (e.g. on https://share.firefox.dev/403suOD ), the
code in Firefox which emits these markers uses the typed marker API
for the IntervalStart marker, and the payload-less API for the IntervalEnd
marker:
https://searchfox.org/mozilla-central/rev/43ce3de32b3a946bceac74c3badf442af9f245c0/tools/profiler/core/platform.cpp#7496-7497
So unpaired end markers got a broken tableLabel from the Awake schema
before this patch ("Awake - CPU Id = " on https://share.firefox.dev/4fx8cSf ).

These two cases are fixed by this patch - the two markers get the
default tableLabel, which does not refer to these absent fields.
RefreshDriverTick markers from Firefox are Text markers.
Firefox (as of Jan 2025) has never emitted a 'RefreshDriverTick' schema.

Only pre-schema profiles have a 'RefreshDriverTick' schema, which they
get from the v33 upgrader.

Post-schema profiles don't display RefreshDriverTick markers in the
timeline-overview area, because Text markers aren't displayed there.

This commit makes the marker test consistent with what Firefox outputs.
Now it no longer relies on a special case in getMarkerSchemaName which
looks up schemas for Text markers based on the marker name.

To test timeline-overview filtering, a new marker is introduced, with
schema name 'VisibleInTimelineOverview'.
Just use data.type, like for other normal markers.

This mostly just affects the RefreshDriverTick marker from upgraded profiles:

 - Pre-schema profiles which get their schema from the v33 upgrader have a
   schema with name 'RefreshDriverTick' with 'timeline-overview' in the display array.
 - Post-schema profiles from Firefox do not have a 'RefreshDriverTick' schema.
   The RefreshDriverTick markers in such profiles is a regular Text marker,
   and Text markers do not show up in the 'timeline-overview' area.

This change makes it so that the 'RefreshDriverTick' schema from
upgraded pre-schema profiles becomes unused, and the markers vanish from
the timeline-overview area.
This is consistent with post-schema profiles.
@mstange mstange force-pushed the use-data-type-for-schema branch from 5295755 to 02732bf Compare January 6, 2025 14:43
Copy link

codecov bot commented Jan 6, 2025

Codecov Report

Attention: Patch coverage is 87.17949% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.59%. Comparing base (b3fe3c5) to head (adca93c).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/profile-logic/processed-profile-versioning.js 76.19% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5293      +/-   ##
==========================================
- Coverage   88.61%   88.59%   -0.03%     
==========================================
  Files         308      308              
  Lines       28125    28134       +9     
  Branches     7621     7625       +4     
==========================================
+ Hits        24923    24924       +1     
- Misses       2987     2994       +7     
- Partials      215      216       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mstange mstange force-pushed the use-data-type-for-schema branch from 02732bf to 05205e3 Compare January 6, 2025 14:54
This adjusts some markers and schemas to be more consistent with
what Firefox outputs:

Markers with { type: "Paint" / "Navigation" / "Layout" } aren't present
in any existing profiles, they were neither emitted by Firefox nor generated
by an upgrader.
Instead, these markers have { type: "tracing" } in their payload.

DOMEvent markers were tracing markers in the past, and have their own
schema today. The test now uses both variants.

CC markers were tracing markers in the past, and have their own schema
today. This test now uses the former.

Post-schema profiles from Firefox have schema with name 'tracing'.
This makes tracing markers visible in the 'timeline-overview' area.
This patch adds the tracing schema to the test profile.

This affects a test which was expecting that 'RandomTracingMarker' was not
visible in the timeline-overview area. The test expectation is adjusted.

Upgraded pre-schema profiles do not have a 'tracing' schema. This will
be fixed in the next commit.
…emas.

This adds an upgrader so that old profiles still get their CC markers
displayed in the memory track and non-CC tracing markers displayed in
the timeline-overview area.

Newer profiles don't need this special case because their CC markers have
a CC schema which correctly puts them in the memory track, and because
they include a 'tracing' schema which puts non-CC tracing markers into
the timeline-overview area.
@mstange mstange force-pushed the use-data-type-for-schema branch from 05205e3 to adca93c Compare January 6, 2025 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

getMarkerSchemaName shouldn't be necessary
1 participant