getMarkerSchemaName shouldn't be necessary

[mstange]

We currently have the following function to match a marker to its schema:

export function getMarkerSchemaName(
  markerSchemaByName: MarkerSchemaByName,
  markerName: string,
  markerData: MarkerPayload | null
): string {
  if (!markerData) {
    // Fall back to using the name if no payload exists.
    return markerName;
  }

  const { type } = markerData;
  if (type === 'tracing' && markerData.category) {
    // TODO - Tracing markers have a duplicate "category" field.
    // See issue #2749

    // Does a marker schema for the "category" exist?
    return markerSchemaByName[markerData.category] === undefined
      ? // If not, default back to tracing
        'tracing'
      : // If so, use the category as the schema name.
        markerData.category;
  }
  if (type === 'Text') {
    // Text markers are a cheap and easy way to create markers with
    // a category. Check for schema if it exists, if not, fallback to
    // a Text type marker.
    return markerSchemaByName[markerName] === undefined ? 'Text' : markerName;
  }
  return type;
}

This is certainly not what I expected. I would have expected the following:

export function getMarkerSchemaName(markerData: MarkerPayload | null): string | null {
  if (markerData && markerData.type) {
    return markerData.type;
  }
  return null;
}

The current code only makes any sense at all for old profiles which got their schemas from an upgrader. New profiles have schemas because someone wrote C++ code to define a schema. Once you have your marker class which defines the schema, you'll then use that class in the profiler_add_marker calls. You wouldn't define the schema and then ignore your class and add a tracing marker or a text marker instead! And even if you did, you'd need to rely on someone else using your class to emit a typed marker - if nobody emits a typed marker using that class, the schema won't make it into the profile!

---

What are the effects of the current implementation of getMarkerSchemaName? What would change if we simplified it as suggested?

Markers with null markerData

I found two marker names which matched schemas for the null data case: CPUSpeed and Awake.

The first CPUSpeed marker on https://share.firefox.dev/4gmLaif has a null data field.
Today it matches a CPUSpeed schema. This gives it a broken tableLabel ("CPUSpeed Speed = GHz").
I can't find any code which emits a marker with name CPUSpeed so I'm thinking this profile was from a work-in-progress Gecko patch.

For Awake (e.g. on https://share.firefox.dev/403suOD ), we appear to use 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 get a broken tableLabel from the Awake schema today ("Awake - CPU Id = " on https://share.firefox.dev/4fx8cSf ).

Tracing markers

Tracing markers have data.type === "tracing" and data.category === "Paint" (or some other category, e.g. "CC", "Navigation", "IPC", and a few more).

Upgraded pre-schema profiles

Pre-schema profiles were upgraded and have schemas named Paint, CC, and Navigation, but no schema named tracing. One such example is https://share.firefox.dev/4iK3005 .

So if we just used data.type to look up the schema, we wouldn't find a schema for the tracing markers on that profile.
This means that the tracing markers with category Paint would no longer show up in the timeline-overview display area.
Those markers have the following names:

• Styles
• Reflow
• RefreshDriverTick
• Rasterize
• ForwardTransaction
• Scripts
• DispatchSynthMouseMove
• DisplayList
• LayerBuilding
• DisplayListResources
• Composite

Furthermore, tracing markers with category CC would no longer show up in the timeline-memory display area. (This profile doesn't have a memory track, but the memory-related markers are still in a distinct subtrack above the timeline-overview markers.)
And these CC markers would no longer have the tooltip Cycle Collect.
The tracing markers with category CC have the following names:

• IdleForgetSkippable
• ForgetSkippable
• IdleCCSlice
• CCSlice

Also, tracing markers with category Navigation would no longer show up in the timeline-overview display area.
These markers have the following names:

• Load
• Unload
• DOMContentLoaded

Post-schema profiles

Post-schema profiles do have a schema with name tracing.

Example: https://share.firefox.dev/4gLxFrQ
The generic tracing schema has timeline-overview in its display.
But these profiles have no schema with name Paint or Navigation.

And modern profiles have a schema with name CC! It is intended for actually typed CC markers though, not for tracking markers of category CC. However, by pure luck, this CC schema still makes it so that tracing markers with category CC will end up in the memory track, for example in this profile: https://share.firefox.dev/3VNPM8w
Modern versions of Firefox don't emit any tracing markers with category CC any more.

Also, there is now a legitimate schema with the name IPC. It is obviously intended for typed IPC markers. However, we also use it for tracing markers with category IPC, and it causes problems:
On https://share.firefox.dev/3BuOoB3, the tracing markers with category IPC have broken tooltips. They try to use the tooltipLabel: "IPC — {marker.data.niceDirection}" from the legitimate IPC schema, but these tracing markers don't have a niceDirection field.

Text markers

There were two types of Text markers I could find which matched a schema by their name: RefreshDriverTick and UserTiming.

UserTiming

The UserTiming marker comes from a ChromeUtils.addProfilerMarker call:

https://searchfox.org/mozilla-central/rev/f009b3332db1c5197b1eb9c7108f68f9bb0900a8/browser/components/newtab/lib/TelemetryFeed.sys.mjs#218-222

    ChromeUtils.addProfilerMarker(
      "UserTiming",
      now,
      "browser-open-newtab-start"
    );

Example profile: https://share.firefox.dev/3P9G4cQ

For this Text marker, we today use the UserTiming schema that exists for actual typed UserTiming markers. The schema happens to somewhat work - both Text markers and legitimate UserTiming markers have a field with name name, so the tooltipLabel and even the field definition ({ "key": "name", "label": "User Marker Name", "format": "string", "searchable": true }) happens to line up somewhat.

But this marker clearly wasn't intended to use the UserTiming schema.

Also, UserTiming is a bad name for this marker.

RefreshDriverTick

The RefreshDriverTick marker is still a plain Text marker today - it doesn't have a schema. But we have an upgrader which adds a RefreshDriverTick schema! The schema from the upgrader is different from the Text marker schema in two ways:

1. It renames the Details field to Tick reasons
2. It has timeline-overview in the display areas.
3. It doesn't provide a tableLabel.

The absence of tableLabel gives these markers an empty column in the marker table for profiles which picked up the RefreshDriverTick schema from the upgrader: https://share.firefox.dev/3VQAJLt

---

Suggested path forward:

I think we shouldn't block on a Gecko change to simplify this.

We can use profile processing as the decarmation between "messy Gecko data" and "pristine profiles".

I suggest the following:

• When markerData === null, don't use any schema. Remove the name fallback without replacement. It's not needed.
• When markerData.type === "Text", always use the Text schema. Remove the name fallback without replacement. It's not needed. The only thing it achieves today is that RefreshDriverTick markers get put in the timeline-overview area, but only for old profiles and not for new profiles. If we really needed these markers in the timeline-overview area we would have noticed their absence by now.
• For tracing markers, do the following:
  • Add a generic tracing schema in a processed profile upgrader to old profiles which don't have one. This will make tracing markers show up in the timeline-overview display area.
  • To get tracing markers with category CC show up in the memory track (e.g. on https://share.firefox.dev/3VNPM8w ), change those markers to use markerData.type === "tracing-CC", and add a tracing-CC schema. Do this both in an upgrader for the processed format, and in an upgrader for the Gecko format, after finding out which Gecko version removed CC tracing markers.
  • Then we can remove the tracing marker special case too, and complete the simplification.

┆Issue is synchronized with this Jira Task

[mstange]

Paper trail: I used the following checker function in combination with the script from PR #5274, on base commit 6fa213a, and checked 1716 different profiles:

function checkProfileSchemaMatching(profile: any, _hash: string): Set<string> {
  const { meta, threads } = profile;
  const { markerSchema } = meta;
  const markerSchemaNames = new Set(markerSchema.map((schema) => schema.name));
  const tracingCategories = new Set();
  const textNames = new Set();
  const noPayloadNames = new Set();

  const outcomes = new Set();
  for (let threadIndex = 0; threadIndex < threads.length; threadIndex++) {
    const thread = threads[threadIndex];
    const { markers, stringTable } = thread;
    for (let markerIndex = 0; markerIndex < markers.length; markerIndex++) {
      const nameIndex = markers.name[markerIndex];
      const data = markers.data[markerIndex];
      const name = stringTable.getString(nameIndex);
      if (
        data &&
        data.type &&
        data.type === 'tracing' &&
        data.category &&
        markerSchemaNames.has(data.category)
      ) {
        const key = name + '#' + data.category;
        if (!tracingCategories.has(key)) {
          console.log(
            `Found tracing marker with name ${name} and category ${data.category}, thread index ${threadIndex}, marker index ${markerIndex}`
          );
          outcomes.add(
            `has tracing marker whose schema is for category ${data.category}`
          );
          tracingCategories.add(key);
        }
        continue;
      }
      if (
        data &&
        data.type &&
        data.type === 'Text' &&
        markerSchemaNames.has(name)
      ) {
        if (!textNames.has(name)) {
          console.log(
            `Found Text marker whose schema is for name ${name}, thread index ${threadIndex}, marker index ${markerIndex}`
          );
          outcomes.add(`has Text marker whose schema is for name ${name}`);
          textNames.add(name);
        }
        continue;
      }
      if (!data && markerSchemaNames.has(name)) {
        if (!noPayloadNames.has(name)) {
          console.log(
            `Found payload-less marker whose schema is for name ${name}, thread index ${threadIndex}, marker index ${markerIndex}`
          );
          outcomes.add(
            `has payload-less marker whose schema is for name ${name}`
          );
          noPayloadNames.add(name);
        }
        continue;
      }
    }
  }

  return outcomes;
}