Add CaptureMask support - #34
Conversation
Change contains: - Ability to call RegisterCaptureMask() for a given name and an optional Telemetry Zone display color - Automatically assign a color if one is not provided Part of: https://fenriscreations.atlassian.net/browse/PLAT-11474
Changed: - enum class Color => CcpColor - namespace CcpColor => ColorUtil
Change includes: - Expose CcpGetRegisteredCaptureMasks() from core - Add/change test coverage for CaptureMasks tests - Rename existing CaptureMasks variables to state they are for "registered" CaptureMasks Part of: https://fenriscreations.atlassian.net/browse/PLAT-11475
Will give us the option of a O(1) lookup for display color once we change telemetry to mark zones based on CaptureMask bit.
Change contains: - Two overloads of CcpSetActiveCaptureMask() function - An exported CcpGetActiveCaptureMask() function - Support for an "all" active CaptureMask - Support for lazy-register of active CaptureMasks by name via a list of pending ones. Part of: https://fenriscreations.atlassian.net/browse/PLAT-11481
Make sure we can verify the color associated with a Zone from the TracyTestClient in tests.
Done because of build problems on v143/v145 vs v141 identified by ccptoebeans.
Add CaptureMaksBit overloads for: - TelemetryZone constructor - CcpTelemetryEnterZone() Make sure tests reflect the new reality. Part of: https://fenriscreations.atlassian.net/browse/PLAT-11476
…pport # Conflicts: # include/CcpTelemetry.h
Rename: CcpGetCaptureMasks() To: CcpGetRegisteredCaptureMasks()
|
|
Legal hasn't advised on an updated statement yet, so we stick with `CCP ehf.`.
…es for a given color value To avoid unnecessary memory copies of the underlying string data, and to avoid surprises in seeing `CcpColorToString( CcpColor::Cyan )` return `Aqua`.
Instead, have `Color` as a default-parameter that is piped through. This alone shouldn't have required a separately overloaded implementation. Also, stop lower-casing the input name, because it causes a mismatch between what is passed into the system as "display name" versus what comes back out of the system. Additionally, remove the `all` special case handling. Enabling all capture masks can be done by passing in all capture masks. Having a sentinel value that represents a possible legal value is bound to cause surprising behaviour down the line. Sentinel values should be flagged explicitly, e.g. an alternative would have been to have an explicit function like `CcpCaptureEverything()` function. Furthermore, this simplifies initialization of pre-registered captureMasks: why run a lambda function during static initialization stage when we have initializer lists?
For type-safe constants instead of preprocessor text replacement.
There is only one capture mask from the user's perspective, so let's keep implementation details out of the function name.
There were two issues at play here: 1. The same iterator into the `FiberNameStore` may have been queued for erasure multiple times 2. Inserting a name into the `FiberNameStore` that has a pending erasure would Surprisingly, those issues primarily surfaced on macOS, and only rarely on Windows. The solution to the problem is to maintain a map instead of a queue. This way, inserting into `FiberNameStore` can cancel any pending erasure. Additionally, the map ensures that there is only ever one entry for `FiberNameStore` iterator that should be deleted.
There was a problem hiding this comment.
There are clear improvements which can be made to the documentation, and the code could do with some refactoring. At a functional level, however, I don't see any problems with the code as it's written.
What I'd like to draw particular attention to is how the API makes mentions of the CaptureMask concept. As the code is written now, a capture mask can be either a struct, a list of strings, a list of structs, when it's in fact a uint64_t type.
Take the following functions for example:
std::vector<CcpCaptureMaskInfo> CcpGetRegisteredCaptureMasks()std::vector<std::string> CcpGetActiveCaptureMask()
There's only ever one capture mask active, yet the API reads like we may have multiple capture masks registered and active within our telemetry integration.
Neither of these functions return a capture mask. They return a list of names, which correspond to a profiling category which may be represented by a capture mask. In that sense, they´re not behaving as their name suggests.
This goes even further, see below:
CcpCaptureMaskHandle CcpRegisterCaptureMask( const std::string& name, CcpColor color = CcpColor::Fuchsia )
This function is not registering a capture mask. It's registering a profiler category which the code is later able to associate with a bit and incorporate into a capture mask.
These examples read like there's something missing at a conceptual level from the code. Or perhaps the functions just need to be renamed. I don't want to state which is which, but I believe this could stand improving.
| // - An active CaptureMask defaults to "all" but can be set/narrowed before or | ||
| // during a Telemetry session is started using either: | ||
| // - a numerical bit mask value of the active CaptureMask | ||
| // - list of "component display names" ("all" is allowed) | ||
| // - Setting active CaptureMask is available for both: | ||
| // - already registered components | ||
| // - "pending" (yet to be registered) components |
There was a problem hiding this comment.
Agreed. We should reserve comment blocks like this for classes and symbols we ourselves define, rather than explaining external concepts like what a capture mask is. Top-down explanations for how our systems operate should be included in general documentation as well.
| // --------------------------------------------------------------------------- | ||
| // CaptureMasks: | ||
| // - CaptureMask(s) are used to determine if a given Zone should be emitted to | ||
| // Telemetry tracking or not based on its origin, i.e. carbon component. |
There was a problem hiding this comment.
There is no hard association between capture masks and carbon components. A component may register multiple capture masks, for example, or even none. This reads a bit confused.
| // - a numerical bit mask value of the active CaptureMask | ||
| // - list of "component display names" ("all" is allowed) | ||
| // - Setting active CaptureMask is available for both: | ||
| // - already registered components |
There was a problem hiding this comment.
This is just a struct. What is this concept of components already registered or pending registration? This documentation may be relevant to whatever function performs the registration, or the container for pending components. But I don't think these are useful concepts to explain in the context of a CcpCaptureMaskInfo struct.
| // --------------------------------------------------------------------------- | ||
| struct CcpCaptureMaskInfo | ||
| { | ||
| std::string name; // The lower-case display name of the CaptureMask (carbon-component) |
There was a problem hiding this comment.
Carbon components are not conceptually related to capture masks. I'm also not sure it's useful to document that the display name is in lower-case, unless the system requires display names to be lowercase, or the display name is converted to lowercase upon registration. Either case, that should probably be documented alongside the registration function for capture masks.
| struct CcpCaptureMaskInfo | ||
| { | ||
| std::string name; // The lower-case display name of the CaptureMask (carbon-component) | ||
| CcpColor color{CcpColor::White}; // The chosen/allocated color for the CaptureMask |
There was a problem hiding this comment.
The words "chosen" and "allocated" are not necessarily interchangeable. Does some external system allocate a display color to the mask info struct? I suggest documenting this with a simpler // Display color, further context can be added to code points utilizing the struct, if necessary.
|
|
||
| bool CcpSetCaptureMask( const std::vector<std::string>& maskNames ) | ||
| { | ||
| // Guard access to all CaptureMasks members |
There was a problem hiding this comment.
| uint64_t newActiveCaptureMask = 0; | ||
| for( const auto& rawName : maskNames ) | ||
| { | ||
| if( rawName.empty() ) |
There was a problem hiding this comment.
I think the code would read better if the contents of this for loop was extracted into a separate function, i.e. bool CcpAddCaptureMask(const std::string& name)
|
|
||
| CARBON_CORE_API uint64_t CcpRegisterCaptureMask( const std::string& name ); | ||
| CARBON_CORE_API uint64_t CcpRegisterCaptureMask( const std::string& name, CcpColor color ); | ||
| CARBON_CORE_API std::vector<CcpCaptureMaskInfo> CcpGetRegisteredCaptureMasks(); |
There was a problem hiding this comment.
Because the array contains reserved entries as well as all the registered capture masks.
Those two tests covered functionality that has been removed / collapsed into the same `TelemetryZone` constructor since then.
As per code-review feedback, the "mask" implementation details does no longer hold up across the API surface.
Reduces compile / link time when building without telemetry, and removes the need for the `#ifdef` in the test code. Take note that currently building `WITH_TELEMETRY=OFF` does not work in general for core, unrelated to this work.
Without it, there's a risk of accessing bad memory when determining the color, and odd behaviour when checking the bit mask.
As per code review feedback. It's a better way to track existence without relying on a sentinel value that could theoretically be valid.
As opposed to the generic `std::string`.
Surprised this didn't crash; it's undefined behaviour, and a debugging session revealed that at least on macOS this access arbitrary memory.
Naming is hard. This avoids confusion with `TelemetryZone`.
ccp-serpent
left a comment
ccp-serpent
left a comment
There was a problem hiding this comment.
The code reads much better. I've requested a minor optimization and one bit of refactoring. The outstanding observations regarding the documentation will also need addressing prior to approval.
| for( const auto& registeredEntry : s_registeredProfilerZones ) | ||
| { | ||
| if( ! registeredEntry->name.empty() ) | ||
| if( registeredEntry ) |
There was a problem hiding this comment.
Since the container is populated from beginning to end, we could break early from this loop when the container entry does not have a value. When that's the case, none of the subsequent elements will have a value either.
There was a problem hiding this comment.
This technique is used in CcpSetActiveProfilerZones, so there's precedent elsewhere in the code for breaking early in this manner.
|
|
||
| CARBON_CORE_API bool CcpSetActiveProfilerZones( const std::vector<std::string>& maskNames ); | ||
| CARBON_CORE_API std::vector<CcpProfilerZone> CcpGetActiveProfilerZones(); | ||
| CARBON_CORE_API bool CcpSetActiveProfilerCategories( const std::vector<std::string>& maskNames ); |
There was a problem hiding this comment.
The maskNames parameter passed in here should be renamed to categoryNames or something similar in order to conform to other parts of the API.
Global search-and-replace is not a great tool.
Because everything runs in the same process, and the network communuication should go over the loopback adapter, the 100ms timeout ought to be more than enough. It can be increased again should need be, but the trade off is overall test execution time: there is no way to test "absence" of a telemetry event (e.g. something has _not_ happened) other than to let the timeout expire.
The motivation here is to prevent users of `CcpTelemetryZone` from being "bad citizens", e.g. passing arbitrarily constructed `CcpProfilerCategory` instances into it. It also unlocks a bunch of further simplifications, among other: - there are significantly fewer copies of `CcpProfilerCategory` going around - the bitmask calculation, while already cheap with the handle abstraction, is now happening only once, upon creation of a category - less sanity checks and lookups required when creating a `CcpTelemetryZone` because no invalid values can be passed in (an "empty" `CcpProfilerCategory` is the worst input, but that simply means such a zone won't be captured) - it removes a "clever workaround" that the handle approach required for the old `TMCM_CPP` / `TMCM_GENERAL` constants
Naming is hard. This is consistent with the rest of the exposed API.
Those functions are either potentially dangerous, limited in functionality, or both.
| return {}; | ||
| } | ||
|
|
||
| bool CcpSetActiveProfilerCategory( const std::vector<std::string>& ) |
There were a few issues with building the documentation: 1. The output location for the documentation was not scoped correctly, and mismatching between what the `Breathe` plugin expected vs. where `doxygen` actually wrote the files to. 2. The doxygen build step failed when the output directory did not yet exist. So now that will get created accordingly. 3. Doxygen was not told about preprocessor macros, so it generated documentation for the wrong code. 4. The default doxygen configuration from the template does not apply here because the repository does not follow the same folder structure. Therefore, the inputs had to be adjusted. 5. Doxygen needs `BUILTIN_STL_SUPPORT` enabled or a tag file. But there is no tag file, so enabling the support it is.
…d of `std::vector<std::string>` This fixes the last remaining inconsistency in the public API and massively simplifies the implementation. It nudges users towards `CcpTelemetryRegisterCategory` / `CcpTelemetryGetRegisteredCategories` as well in order to get the required information.
This has been broken for a long time. There is an argument to remove that option since it is never built on CI. But until such a decision is made, this should work, too.
Curious that this went unnoticed until now. Turns out, there was no test for the new constructor.
AI generated, needs some refinement.
Missing preprocessor definition forwarding and lack of their resolution by doxygen caused a lot of confusion for Sphinx' "Breathe" plugin. "Breathe" was also not configured for `.cpp` files. Furthermore, setting doxygen to `EXTRACT_ALL` leaks a lot of internal symbols into the documentation, some of them which confuse Breathe, like the double occurrence of various static variables.
Similar argument as for disabling `EXTRACT_ALL`: There is a lot of noise in the documentation. Additionally, Breathe complains about duplicate symbols because it will see some of them in two separate XML files generated by doxygen.
Minor edits to improve wording and strip out unnecessary details.
While it is currently a 64 bit mask that is utilizied underneath the hood, then that is no useful information for the user of telemetry, as the bitmask is never exposed. Everything is abstracted away and hidden behind a `CcpTelemetryCategory`
5 participants
Change includes: