WIP share more between threads #5273
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mstange
force-pushed
the
push-wsuwrvunnvmr
branch
2 times, most recently
from
a5c010a to
f036db9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5273 +/- ##
==========================================
- Coverage 86.07% 86.04% -0.04%
==========================================
Files 311 311
Lines 29657 30153 +496
Branches 8196 8262 +66
==========================================
+ Hits 25528 25944 +416
- Misses 3547 3617 +70
- Partials 582 592 +10 ☔ View full report in Codecov by Sentry. |
mstange
force-pushed
the
push-wsuwrvunnvmr
branch
from
f036db9 to
5f8c157
Compare
mstange
force-pushed
the
push-wsuwrvunnvmr
branch
3 times, most recently
from
e016a6d to
60854ff
Compare
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.
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.
…om getSchemaFromMarker.
This removes the distinction between the "serializable" and the "unserialized" / in-memory format. The raw profile is now always serializable to JSON. The format stays unchanged - samples and counter samples are still allowed to have their timestamps stored as either timestamps or as time deltas. We now convert the samples and counter samples into the timeDeltas format during processing and in the upgrader. This matches the previous behavior where we did this conversion during profile serialization.
mstange
force-pushed
the
push-wsuwrvunnvmr
branch
2 times, most recently
from
2735b45 to
3779d4d
Compare
Stop storing them in the file, compute them at runtime. This reduces profile sizes.
mstange
force-pushed
the
push-wsuwrvunnvmr
branch
from
3779d4d to
eaa6409
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.