Closed Bug 1670954 Opened 4 years ago Closed 4 years ago

The new marker API needs a way to record a marker on the main thread track

Categories

(Core :: Gecko Profiler, task, P2)

task

Tracking

()

RESOLVED FIXED
83 Branch
Tracking Status
firefox83 --- fixed

People

(Reporter: florian, Assigned: mozbugz)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

profiler_add_marker_for_mainthread from the old API made it simple to record markers on the main thread. There's currently no equivalent in the new API. MarkerThreadId makes it easy to record a marker into another thread if the id of the other thread is known, but it would be useful to have a simple API to send the marker to the main thread (the id of the main thread is usually not available when recording a marker from another thread).

A simple idea of API would be MarkerThreadId::MainThread() in the marker options.

Thinking further, profiler_add_marker_for_mainthread currently has only one caller: the IO markers. And other things the I/O markers do could be worth generalizing:

  • recording the thread ID of the thread where the marker was added
  • adding the suffix to indicate that a marker was from "non-main thread", "non-profiled thread", "unregistered thread".

So I'm wondering if we could have a marker option to show the marker on the main thread, that would automatically:

  • record the marker on both the thread where the thing occured and the main thread (if different)
  • add the relevant suffix to the marker name on the main thread
  • record the thread ID.

https://hg.mozilla.org/try/rev/99c581cae1fc8b73124a1f79fdf7f0ac6c5502b7 shows an example case of markers where this behavior would be very nice.

The native allocations also record to the main thread, but we don't want them on the originating thread.

https://searchfox.org/mozilla-central/rev/d25eb00ab4e90cc0130cd18f303a04cc2a2f8409/tools/profiler/core/platform.cpp#5570

Good idea about MarkerThreadId::MainThread(), I'll work on that here soon.

About the other (good) suggestions, they are less obvious.

Since we don't have a common mechanism for recording a different thread id (than where the marker is stored in and displayed), the file IO and native allocations record the original thread id as part of their payload, and there is some post-processing happening in the front-end (to show the thread name when known).
We could possibly make that common (i.e., add it to the common fields outside the payload) but I'm not sure it's worth it?
Another option would be to keep this a payload field, but there could be a new marker schema format (e.g., MarkerSchema::Format::thread), so that any payload can have any number of other thread ids, with a proper display on the frontend.

And adding suffixes seems even more case-specific.

To be discussed further. I believe these extra options don't need to be done immediately to make "markers 2.0" complete, so we can take the time to think about all this...

Assignee: nobody → gsquelart
Severity: -- → N/A
Priority: -- → P2

As for the long-term plan, I think we should always capture (and include in the JSON) all markers from all threads. And then we can display them in the front-end in whichever way we want, probably controlled by the marker schema in some way. For example, for "NewThread" markers, the marker type could declare itself as "global" in the marker schema. And the front-end could then decide to display "global" markers on the main thread, rather than on the thread that emitted the marker. Or in both threads, as suggested in comment 0.

(In reply to Markus Stange [:mstange] from comment #3)

As for the long-term plan, I think we should always capture (and include in the JSON) all markers from all threads.

Haha, I was thinking about this last night 😄
It'd be great, but my worry is that we then wouldn't control the memory used by all markers that are not in the list of threads in about:profiling, so that could shorten the effective profiling window, especially when we invite devs to add more markers everywhere... 🤔
(Maybe we'll eventually need to pre-filter markers in other ways, e.g., by category?)

... the marker type could declare itself as "global" in the marker schema

This could be a solution to my worry: Only a few markers could declare themselves as "global" and only they would always be recorded.

I wouldn't limit all-thread marker capturing to "global" markers. I think filtering by category would be a better way to combat excessive buffer use. Maybe it is time to revive the work done in bug 1369955.

These functions will be useful to get the main thread id, or check if we're in it, in some public code (e.g., markers).

Since the main thread is almost always being profiled, this makes it easy to direct important markers there (e.g., FileIO markers from unregistered threads).

Tech note: The #include "BaseProfiler.h" line was moved to BaseProfilerMarkersPrerequesites.h so that MarkerThreadId::MainThread() there can access profiler_main_thread_id().

Depends on D93735

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a01db300cf22
profiler_main_thread_id() and profiler_is_main_thread() - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/e582038fdae2
MarkerThreadId::MainThread() - r=gregtatum
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 83 Branch
Blocks: 1672256
Regressions: 1672501
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: