Skip to content

Give ProfilerManager access to the State being profiled (#2234) - #2277

Closed
devtejasx wants to merge 1 commit into
google:mainfrom
devtejasx:profiler-manager-state
Closed

devtejasx wants to merge 1 commit into
google:mainfrom
devtejasx:profiler-manager-state

Conversation

@devtejasx

@devtejasx devtejasx commented Aug 1, 2026 •

Copy link
Copy Markdown
Contributor

Custom profilers that open a named region need the benchmark name, but the ProfilerManager hooks receive no context at all.

Adding an overload of AfterSetupStart()/BeforeTeardownStop() that takes a State is not source compatible: the new overload hides the old one in every existing subclass. Instead, add a protected ProfilerManager::GetState() accessor that returns the State of the run being profiled. Existing subclasses keep compiling untouched.

The accessor returns a pointer, not a reference, because there is no meaningful answer outside the hooks. State sets the pointer immediately before invoking a hook and clears it as soon as the hook returns, so a caller outside AfterSetupStart()/BeforeTeardownStop() observes nullptr rather than a stale State. The test asserts both: the benchmark name is readable inside the hook, and the accessor is null outside it.


Per AGENTS.md: AI-assisted — the patch was drafted with AI assistance
(Claude) and then reviewed, tested, and understood by me. I take full
responsibility for it.

Custom profilers that open a named region need the benchmark name, but the
ProfilerManager hooks receive no context at all.

Adding an overload of AfterSetupStart()/BeforeTeardownStop() that takes a
State is not source compatible: the new overload hides the old one in every
existing subclass. Instead, add a protected ProfilerManager::GetState()
accessor that returns the State of the run being profiled. Existing
subclasses keep compiling untouched.

The accessor returns a pointer, not a reference, because there is no
meaningful answer outside the hooks. State sets the pointer immediately
before invoking a hook and clears it as soon as the hook returns, so a
caller outside AfterSetupStart()/BeforeTeardownStop() observes nullptr
rather than a stale State. The test asserts both: the benchmark name is
readable inside the hook, and the accessor is null outside it.
@dmah42 dmah42 closed this Aug 3, 2026
@devtejasx

Copy link
Copy Markdown
Contributor Author

Closed without a comment, so I can't tell what to change. Was it the approach — a protected GetState() accessor with the State reachable only inside the two hooks — or the feature itself?

If it is the approach, the alternative that avoids both problems raised on the earlier attempts is a differently named hook rather than an overload: AfterSetupStart(const State&) hid the existing AfterSetupStart() in subclasses, which is what made #2249 source-incompatible, but a new name with a default implementation that forwards to the old one does not. Happy to send that instead.

If the feature isn't wanted, that's a fine answer too — #2234 can be closed and I'll stop pushing on it.

@dmah42

dmah42 commented Aug 5, 2026

Copy link
Copy Markdown
Member

there's no motivation for the feature, as far as i can tell. noone is asking for it, the issue doesn't explain why someone would want to do it.

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