Skip to content

Change the marker schema to accept a description and get rid of the notion of static fields#5385

Merged
mstange merged 2 commits into
firefox-devtools:mainfrom
mstange:description-instead-of-static-fields
Feb 28, 2025
Merged

Change the marker schema to accept a description and get rid of the notion of static fields#5385
mstange merged 2 commits into
firefox-devtools:mainfrom
mstange:description-instead-of-static-fields

Conversation

@mstange

@mstange mstange commented Feb 25, 2025

Copy link
Copy Markdown
Contributor

In a marker schema, the data array can currently hold both "dynamic" and "static" fields. Dynamic fields get their value from each marker, static fields have a single fixed value.

In the years since the marker schema has been available, the only thing we've used "static" fields for is a "description" field.

So I think we should just have an actual description field which is separate from schema.data. And we should remove generic "static" fields. This simplifies the format, and it also simplifies our code because we can iterate over the fields without having to check whether a field is a static or a dynamic field - Flow was requiring extra verbose checks in this case because the two field types don't have a shared discriminator property.

This change does make the schema a little less expressive, but nobody was making use of the extra expressivity.

Also, at some point we'll want to move to a more compact representation of markers in the profile JSON by not repeating the field keys on every marker. One way to do this would be to store marker field values in an array, where each array value maps to the field at the corresponding index. This representation makes more sense if the fields array only contains fields that actually get their value from the marker.

This PR only changes the processed format. The Gecko profile format remains unchanged. We can change it in a follow-up.

On the Gecko side, we currently have an old and a new way to declare marker schemas. The new way got rid of static fields too and just accepts a description (but still outputs the description as a static field).

@mstange mstange requested a review from canova February 25, 2025 21:26
@mstange mstange force-pushed the description-instead-of-static-fields branch from ebcdbd2 to a697c2f Compare February 25, 2025 21:27
@mstange mstange changed the title Description instead of static fields Change the marker schema to accept a description and get rid of the notion of static fields Feb 25, 2025
@mstange mstange force-pushed the description-instead-of-static-fields branch from a697c2f to 85bd298 Compare February 25, 2025 21:29
@codecov

codecov Bot commented Feb 25, 2025

Copy link
Copy Markdown

Codecov Report

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

Project coverage is 86.03%. Comparing base (2eb2bf6) to head (85bd298).

Files with missing lines Patch % Lines
src/profile-logic/processed-profile-versioning.js 85.71% 3 Missing ⚠️
src/profile-logic/process-profile.js 92.00% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5385   +/-   ##
=======================================
  Coverage   86.03%   86.03%           
=======================================
  Files         312      312           
  Lines       30318    30350   +32     
  Branches     8288     8291    +3     
=======================================
+ Hits        26084    26112   +28     
- Misses       3640     3644    +4     
  Partials      594      594           

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

@canova canova left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks.

It makes sense to me overall, considering that we are not using for something other than the description. But do we know if that's also the case for external tools that export our data, like vernier? I hope that won't affect others.

Comment thread src/types/markers.js
Comment thread src/test/components/__snapshots__/MarkerChart.test.js.snap
Comment thread src/profile-logic/processed-profile-versioning.js
@mstange

mstange commented Feb 28, 2025

Copy link
Copy Markdown
Contributor Author

I've run the batch-checker over this and the only warnings from this I found were the ones about "Marker: UserTiming". But my list of profile hashes probably doesn't include a lot of third-party generated profiles.

So now I've gone ahead and checked a few profile generators:

@mstange mstange force-pushed the description-instead-of-static-fields branch 2 times, most recently from 448c63b to 0ca0522 Compare February 28, 2025 16:32
…rsion 33.

When the test was initially added, the markerSchema was missing from the profile.
That was invalid because version 33 is the version which started requiring that
the markerSchema field is present.

In firefox-devtools#4352 we added the missing markerSchema because the upgrader for
version 44 caught the fact that it was missing, because it (rightfully) assumes
that it's present.

However, since this is a profile of a fixed version, we shouldn't attempt to
share code with other tests - anything that's shared will usually follow the
most recent format version, whereas this test must make sure the profile it
creates complies with the hardcoded fixed version.
Use an optional description field instead.

This causes less trouble with our Flow types. It's also a clearer API.
And it will make it easier to switch marker.data from being an object
to it being an array, in field order, because we'll be able to say that
there should be one array element per field.
@mstange mstange force-pushed the description-instead-of-static-fields branch from 0ca0522 to b039cd7 Compare February 28, 2025 16:34
@mstange mstange enabled auto-merge February 28, 2025 16:34
@mstange mstange merged commit d42d0f6 into firefox-devtools:main Feb 28, 2025
@julienw julienw mentioned this pull request Mar 3, 2025
julienw added a commit that referenced this pull request Mar 3, 2025
[Julien Wajsberg] Update node to v22 (#5378)
[Markus Stange] Speed up createCallNodeTable by 2.3x (#5248)
[Markus Stange] Remove frameTable.implementation from the processed format. (#5370)
[Florian Quèze] Show size units in the timeline for profiles where profile.meta.sampleUnits.time is "bytes". (#5364)
[Sean Kim] Report nsIRequest::status (nsresult) in the marker (#5375)
[Markus Stange] Change the marker schema to accept a description and get rid of the notion of static fields (#5385)

Also thanks to our localizers:
en-CA: Paul
tr: Grk
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.

2 participants