Closed Bug 1646266 Opened 4 years ago Closed 4 years ago

Implement new C++ way to define marker types

Categories

(Core :: Gecko Profiler, task, P2)

task

Tracking

()

RESOLVED FIXED
82 Branch
Tracking Status
firefox82 --- fixed

People

(Reporter: mozbugz, Assigned: mozbugz)

References

(Depends on 2 open bugs, Blocks 1 open bug)

Details

Attachments

(20 files, 1 obsolete file)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

We want to make it easier to define new types of markers from the C++ side.
(The front-end dependency will be handled by other bugs, in particular the formatting schema, see bug 1640999.)

Currently developers need to create a derived class from ProfilerMarkerPayload, and define functions to:

  • Construct a payload.
  • Compute the serialization size.
  • Serialize to the profile buffer, including taking care to serialize common properties in the base class.
  • Deserialize from the profile buffer, including reconstructing the payload.
  • Stream to JSON.

With only a developer-provided stream-to-json function, I believe it is possible to automatically handle everything else.
For more information and a proof-of-concept, see the "C++ Gecko code" section in https://docs.google.com/document/d/1CCvTL2O_9YF7sd6ef8pWvm23EVgArOsmoNG0zqXFk9w/edit#heading=h.6nbh7k46bsm1

One thing to consider is a simpler option to capture backtraces when adding a marker. In most cases the backtrace is done on the spot, in which case it should be possible to store its data on the stack and avoid an allocation for that (see bug 1578792).

Also, it should be possible to avoid actually constructing a marker structure on the stack; This would effectively "fix" bug 1636802.

See Also: → 1636802, 1578792

This patch introduces all new files that contain the new markers C++ API and implementation.
They are mostly empty at this time, only including each other as eventually needed, and with #ifdef MOZ_GECKO_PROFILER guards.

Rough inclusion diagram: (header <-- includer)

BaseProfilerMarkerPrerequesites.h <-- ProfilerMarkerPrerequesites.h (Useful types: Input string view, marker options)
^ ^
BaseProfilerMarkerDetail.h <-- ProfilerMarkerDetail.h (Implementation details)
^ ^
BaseProfilerMarkers.h <-- ProfilerMarkers.h (Main API)
^ ^--------- ^ ^---------
BaseProfilerMarkerTypes.h | <-- ProfilerMarkerTypes.h | (Common marker types)
^ ^-- BaseProfiler.h <-- GeckoProfiler.h (Existing main profiler headers)

Depends on D87240

These are similar to std::string_view, but they are optimized to be serialized, and later deserialized and streamed to JSON.
They accept literal strings, and keep them as unowned raw pointers and sizes.
They also accept any substring reference, assuming that they will only be used as function parameters, and therefore the dependent string will live during the function call where these StringView's are used.

Internally, they also allow optional string ownership, which is only used during deserialization and streaming.
This is hidden, so that users are not tempted to use potentially expensive string allocations during profiling; it's only used after profiling, so it's less of an impact to allocate strings then. (But it could still be optimized later on, as part of bug 1577656.)

Depends on D87241

This marker option captures a given thread id.
If left unspecified (by default construction) during the add-marker call, the current thread id will be used then.

Depends on D87242

This moves the existing MarkerTiming class introduced in bug 1640969 to the BaseProfilerMarkersPrerequesites.h header, and can be used as a marker option.

Some minor clarifying changes:

  • Instant() is split into two functions: InstantNow() and InstantAt(TimeStamp).
  • Interval(TimeStamp, TimeStamp) must be given both start and end, otherwise IntervalUntilNowFrom(TimeStamp) takes the start only and ends "now".
  • IsUnspecified() indicates if MarkerTiming was default-constructed with no timestamps at all; In this case the add-marker code will replace it with InstantNow().

The serialization only contains the phase, and one or two timestamps as needed, to save space for non-interval timings.

Depends on D87244

IsEmpty() makes it easier for users to determine if there is anything stored in the ProfileChunkedBuffer.

And ProfileChunkedBuffer can now be serialized from a raw pointer, which will be useful when adding markers with an optional stack.
Deserialization should be done to a UniquePtr<ProfileChunkedBuffer>, which is already implemented.

Depends on D87245

This marker option allows three cases:

  • By default, no stacks are captured.
  • The caller can request a stack capture, and the add-marker code will take care of it in the most efficient way.
  • The caller can still provide an existing backtrace, for cases where a marker reports something that happened elsewhere.

Depends on D87246

This option can take an inner window id.

Depends on D87247

A MarkerOptions must be constructed with a known sub-category from ProfilingCategoryList.h, e.g.: MarkerOptions::OTHER(), this will be required for all markers.
MarkerOptions also contains defaulted MarkerThreadId, MarkerTiming, MarkerStack, and MarkerInnerWindowId. They may be changed by chaining Set() functions, e.g.:
PROFILER_MARKER<...>("name", MarkerOptions::Other().Set(MarkerThreadId(otherThread)).Set(MarkerStack::Capture()), ...)

Depends on D87248

This is similar to the existing deserializer table in ProfilerMarkerPayload.*:
Each type of marker (except in the NoPayload one) has its corresponding deserializer in a table, and the index is used as a tag in the serialization buffer.

Depends on D87249

Marker types will be defined with a simple struct that contains (amongst other things) one Stream(JSONWriter&, ...) function.
This patch introduces a type-traits-like helper to examine that function. This will be used to check the arguments given to the upcoming add-marker function, to serialize and deserialize them.

Depends on D87250

NoPayload will be mostly used internally when adding markers without payload data.
It has an empty specialization of the MarkerTypeHelper (mainly to catch misuses), and the add-marker code will need to have different compile-time paths to handle it.

Depends on D87251

A new entry kind is needed to serialize the new markers, because they are not encoded the same way.

Depends on D87252

DeserializeAfterKindAndStream() is the main function that extracts all the marker data (past the already-read entry kind), and streams it to JSON using the user-provided Stream(JSONWriter&, ...) function in the appropriate marker type definition.

It currently requires two external functions to stream the name and the optional backtrace, because these are different between the two profilers. This may change in the future.

(Deserialization is implemented before serialization, because the Deserialize() function is needed during serialization to get a marker type tag.)

Depends on D87253

Main marker serialization function, which accepts the same arguments as the marker type's Stream(JSONWriter&, ...) function, and stores them after the marker name and options.

Depends on D87254

The upcoming profiler-specific add-marker function will need to know which ProfileChunkedBuffer to serialize to, these "detail" functions provide access.

Depends on D87255

This is the main public marker API:

  • AddMarkerToBuffer can be used to store a marker to any buffer. This could be useful to code that wants to store markers outside of the default profiler buffers.
  • AddMarker/profiler_add_marker store a marker to the appropriate profiler buffer.
  • BASE_PROFILER_MARKER and PROFILER_MARKER do the same, but are also available (and empty) when MOZ_GECKO_PROFILER is not #defined.
    All these take a name, marker options, a marker type, and the type's expected arguments if any.

Extra helpers for the most common types:

  • BASE_PROFILER_MARKER_UNTYPED and PROFILER_MARKER_UNTYPED store a marker with no data payload.
  • BASE_PROFILER_MARKER_TEXT and PROFILER_MARKER_TEXT store a text marker. baseprofiler::markers::Text shows how new marker types can be defined.

Depends on D87256

RAII class that may be used to capture a time interval in a marker.
To stay efficient, it requires some awkward lambda work in the call.
Users should instead store a start timestamp and later add a marker with an interval from that start timestamp; but in some cases (e.g., multiple return paths) this Auto class may be more appropriate.

Depends on D87257

This ports existing ProfilerMarkerPayload types to the new API. This is just a starting point, they may be changed later on as needed.

Depends on D87258

Attachment #9170347 - Attachment description: Bug 1646266 - Cached{,Base}CoreBuffer - → Bug 1646266 - Detail functions to access CorePS members -
Attachment #9170344 - Attachment description: Bug 1646266 - Markers Deserialization - → Bug 1646266 - Marker Deserialization -
Attachment #9170330 - Attachment description: Bug 1646266 - ProfilerMarkers skeleton files - → Bug 1646266 - ProfilerMarkers skeleton files - r?gregtatum
Attachment #9170331 - Attachment description: Bug 1646266 - ProfilerMarkers {Base,Marker}StringView - → Bug 1646266 - ProfilerString{,8,16}View - r?gregtatum
Attachment #9170347 - Attachment description: Bug 1646266 - Detail functions to access CorePS members - → Bug 1646266 - CorePS buffer access - r?gregtatum
Attachment #9170333 - Attachment description: Bug 1646266 - Marker option: MarkerThreadId - → Bug 1646266 - Marker option: MarkerThreadId - r?gregtatum
Attachment #9170334 - Attachment description: Bug 1646266 - Marker option: MarkerTiming - → Bug 1646266 - Marker option: MarkerTiming - r?gregtatum
Attachment #9170335 - Attachment description: Bug 1646266 - ProfileChunkedBuffer::IsEmpty() and Serializer<ProfileChunkedBuffer*> - → Bug 1646266 - ProfileChunkedBuffer::IsEmpty() and Serializer<ProfileChunkedBuffer*> - r?gregtatum
Attachment #9170336 - Attachment description: Bug 1646266 - Marker option: MarkerStack - → Bug 1646266 - Marker option: MarkerStack - r?gregtatum
Attachment #9170337 - Attachment description: Bug 1646266 - Marker option: MarkerInnerWindowId - → Bug 1646266 - Marker option: MarkerInnerWindowId - r?gregtatum
Attachment #9170338 - Attachment description: Bug 1646266 - MarkerOptions - → Bug 1646266 - MarkerOptions - r?gregtatum
Attachment #9170339 - Attachment description: Bug 1646266 - Deserializers and Tags - → Bug 1646266 - Deserializers and Tags - r?gregtatum
Attachment #9170340 - Attachment description: Bug 1646266 - MarkerType::Stream function helpers - → Bug 1646266 - MarkerType::Stream function helpers - r?gregtatum
Attachment #9170341 - Attachment description: Bug 1646266 - NoPayload default type, with specialized empty helper - → Bug 1646266 - NoPayload default type, with specialized empty helper - r?gregtatum
Attachment #9170342 - Attachment description: Bug 1646266 - ProfileBufferEntryKind::Marker - → Bug 1646266 - ProfileBufferEntryKind::Marker - r?gregtatum
Attachment #9170344 - Attachment description: Bug 1646266 - Marker Deserialization - → Bug 1646266 - Marker Deserialization - r?gregtatum
Attachment #9170346 - Attachment description: Bug 1646266 - AddMarkerToBuffer - → Bug 1646266 - AddMarkerToBuffer - r?gregtatum
Attachment #9170348 - Attachment description: Bug 1646266 - {,BASE_}PROFILER_MARKER{,_TEXT} - → Bug 1646266 - {BASE_,}PROFILER_MARKER{,_TEXT} - r?gregtatum
Attachment #9170349 - Attachment description: Bug 1646266 - Auto{,Base}ProfilerMarker - → Bug 1646266 - Auto{,Base}ProfilerMarker - r?gregtatum
Attachment #9170350 - Attachment description: Bug 1646266 - {,Base}ProfilerMarkerTypes.h - → Bug 1646266 - {,Base}ProfilerMarkerTypes.h - r?gregtatum
Attachment #9170351 - Attachment description: Bug 1646266 - ProfilerMarkers tests - → Bug 1646266 - Profiler Markers 2.0 tests - r?gregtatum
Attachment #9170349 - Attachment is obsolete: true

The main, and only compulsory, marker option is the MarkerCategory, which captures the "category pair", as well as the parent category.

Depends on D87256

profiler_capture_backtrace(ProfileChunkedBuffer&) renamed to profiler_capture_backtrace_into(ProfileChunkedBuffer&) (notice "_into"), which is clearer.
New function profiler_capture_backtrace() returns a UniquePtr<ProfileChunkedBuffer>, which can later be given to MarkerStack.

profiler_get_backtrace() (returning a UniqueProfilerBacktrace) now uses profiler_capture_backtrace().
This reduces most duplicate code between these functions.

Depends on D87246

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f05f6a52bd7e
ProfilerMarkers skeleton files - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/35166588a684
ProfilerString{,8,16}View - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/e01bc772d669
CorePS buffer access - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/c25cdf173894
Marker option: MarkerCategory - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/9001175e7475
Marker option: MarkerThreadId - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/1c1501cfa02b
Marker option: MarkerTiming - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/df82ad015f84
ProfileChunkedBuffer::IsEmpty() and Serializer<ProfileChunkedBuffer*> - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/4189499ea599
Rework backtrace-capture functions - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/08dd6220f0dd
Marker option: MarkerStack - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/1a17cf6e6b4d
Marker option: MarkerInnerWindowId - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/e482a2568f27
MarkerOptions - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/7ecc9ee961b6
Deserializers and Tags - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/ecfc47fc2444
MarkerType::Stream function helpers - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/d193a9f84702
NoPayload default type, with specialized empty helper - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/9a1cd7b6c1ca
ProfileBufferEntryKind::Marker - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/fa6a89f9eba2
Marker Deserialization - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/9bc0276c9260
AddMarkerToBuffer - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/5e8954913748
{BASE_,}PROFILER_MARKER{,_TEXT} - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/c797da0d5b1b
{,Base}ProfilerMarkerTypes.h - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/0871a6eb61bb
Profiler Markers 2.0 tests - r=gregtatum
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c9ceb8f8dc8
ProfilerMarkers skeleton files - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/ddec14555d7e
ProfilerString{,8,16}View - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/092c89f164ba
CorePS buffer access - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/220f96d1e3a2
Marker option: MarkerCategory - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/be8fd5f6d335
Marker option: MarkerThreadId - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/db3bdff5e4d7
Marker option: MarkerTiming - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/9eff1a8c358e
ProfileChunkedBuffer::IsEmpty() and Serializer<ProfileChunkedBuffer*> - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/de4d9ab1a6e1
Rework backtrace-capture functions - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/d091750b1b14
Marker option: MarkerStack - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/3c1f350a07d5
Marker option: MarkerInnerWindowId - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/678a7c5d8a83
MarkerOptions - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/8e65fa28b768
Deserializers and Tags - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/60a54b5d7bad
MarkerType::Stream function helpers - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/33da5fe6d185
NoPayload default type, with specialized empty helper - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/1eba69baac1f
ProfileBufferEntryKind::Marker - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/d5a7d5139d59
Marker Deserialization - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/da8ae4c7615c
AddMarkerToBuffer - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/119344e72ed8
{BASE_,}PROFILER_MARKER{,_TEXT} - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/5d8691cb0edb
{,Base}ProfilerMarkerTypes.h - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/e2e161965ad3
Profiler Markers 2.0 tests - r=gregtatum
Flags: needinfo?(gsquelart)

Thank you Narcis and Cristian for the backouts.

Please note that I did test on Try first, but didn't see these errors there. I filed bug 1662328 about the differing build results.

I'm working on fixing these errors here again, I'll avoid literal string pointers this time...

Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c38607ea1ba
ProfilerMarkers skeleton files - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/790ed86c1a6c
ProfilerString{,8,16}View - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/3d0160207591
CorePS buffer access - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/25f8e4b67b32
Marker option: MarkerCategory - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/4ea14ca3d492
Marker option: MarkerThreadId - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/22bdc0172356
Marker option: MarkerTiming - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/dfd7e13e9e0b
ProfileChunkedBuffer::IsEmpty() and Serializer<ProfileChunkedBuffer*> - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/bde14a8b0660
Rework backtrace-capture functions - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/3b88d838b252
Marker option: MarkerStack - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/9fd6a871374f
Marker option: MarkerInnerWindowId - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/2c8828fab7a0
MarkerOptions - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/a01a466e464c
Deserializers and Tags - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/b51bc775d1e3
MarkerType::Stream function helpers - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/5e7a28a727a1
NoPayload default type, with specialized empty helper - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/6c2b59568703
ProfileBufferEntryKind::Marker - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/b4a39ef38261
Marker Deserialization - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/fcf3c271d0fc
AddMarkerToBuffer - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/6b71d7b09641
{BASE_,}PROFILER_MARKER{,_TEXT} - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/a0c2db6f73c7
{,Base}ProfilerMarkerTypes.h - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/a2734d73264c
Profiler Markers 2.0 tests - r=gregtatum
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5fffd9f8649
ProfilerMarkers skeleton files - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/b511ca601b74
ProfilerString{,8,16}View - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/6af847204dc5
CorePS buffer access - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/05dd7a1c5279
Marker option: MarkerCategory - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/baaa8d69cf5d
Marker option: MarkerThreadId - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/19230cc586cf
Marker option: MarkerTiming - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/764280d95b6f
ProfileChunkedBuffer::IsEmpty() and Serializer<ProfileChunkedBuffer*> - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/611e97a5af26
Rework backtrace-capture functions - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/ececccb1c743
Marker option: MarkerStack - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/89cdbaea408a
Marker option: MarkerInnerWindowId - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/476acea9fa53
MarkerOptions - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/8f8d2845d089
Deserializers and Tags - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/9ee79e7bd226
MarkerType::Stream function helpers - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/8581a81b51fa
NoPayload default type, with specialized empty helper - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/ac8060521a4f
ProfileBufferEntryKind::Marker - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/75543e56e2e8
Marker Deserialization - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/300dcabd8b53
AddMarkerToBuffer - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/9a2249f099bb
{BASE_,}PROFILER_MARKER{,_TEXT} - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/17bc1530415f
{,Base}ProfilerMarkerTypes.h - r=gregtatum
https://hg.mozilla.org/integration/autoland/rev/44ed92e28c34
Profiler Markers 2.0 tests - r=gregtatum
Flags: needinfo?(gsquelart)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: