Exclude setup/teardown from MemoryManager measurement window (#2149)#2247
Open
devtejasx wants to merge 2 commits into
Open
Exclude setup/teardown from MemoryManager measurement window (#2149)#2247devtejasx wants to merge 2 commits into
devtejasx wants to merge 2 commits into
Conversation
…2149) RunMemoryManager called memory_manager->Start() before allocating the internal ThreadManager and running the benchmark's Setup(), and called Stop() after resetting the ThreadManager and running Teardown(). As a result the MemoryManager attributed the library's own bookkeeping allocation and any per-benchmark fixture allocations to the benchmark, so the reported allocation count could never be accurate. Setup()/Teardown() already run outside the timed region; make memory measurement consistent by bracketing only RunInThread with Start()/ Stop(), and move the ThreadManager creation and Setup()/Teardown() outside that window. Add memory_manager_ordering_test, which registers a MemoryManager and asserts no Setup/Teardown occurs between Start() and Stop(). It fails on the previous ordering and passes with this change. Signed-off-by: devtejasx <tejasnagmote520@gmail.com>
LebedevRI
reviewed
Jul 13, 2026
Comment on lines
+54
to
+67
| auto start = std::find(events.begin(), events.end(), "Start"); | ||
| auto stop = std::find(events.begin(), events.end(), "Stop"); | ||
| assert(start != events.end() && "MemoryManager::Start was never called"); | ||
| assert(stop != events.end() && "MemoryManager::Stop was never called"); | ||
| assert(start < stop && "Start must precede Stop"); | ||
|
|
||
| // Nothing between Start and Stop may be a Setup or Teardown: those run | ||
| // outside the measured window (this is what #2149 fixes). | ||
| for (auto it = start + 1; it != stop; ++it) { | ||
| assert(*it != "Setup" && | ||
| "Setup ran inside the MemoryManager Start/Stop window"); | ||
| assert(*it != "Teardown" && | ||
| "Teardown ran inside the MemoryManager Start/Stop window"); | ||
| } |
Collaborator
There was a problem hiding this comment.
We can't rely on assert for correctness checks, this should be a gtest-based test.
Contributor
Author
There was a problem hiding this comment.
Done — replaced the assert-based test with a proper gtest (memory_manager_ordering_gtest.cc, wired up via add_gtest). All checks are EXPECT_* now.
LebedevRI
reviewed
Jul 13, 2026
Comment on lines
+16
to
+31
| // Ordered log of lifecycle events. All of these callbacks run on the main | ||
| // thread (thread_index 0), so no synchronization is needed. | ||
| std::vector<std::string> events; | ||
|
|
||
| class OrderingMemoryManager : public benchmark::MemoryManager { | ||
| public: | ||
| void Start() override { events.emplace_back("Start"); } | ||
| void Stop(Result& result) override { | ||
| events.emplace_back("Stop"); | ||
| result.num_allocs = 0; | ||
| result.max_bytes_used = 0; | ||
| } | ||
| }; | ||
|
|
||
| void DoSetup(const benchmark::State&) { events.emplace_back("Setup"); } | ||
| void DoTeardown(const benchmark::State&) { events.emplace_back("Teardown"); } |
Collaborator
There was a problem hiding this comment.
Instead of collecting strings,
can we just have a boolean for MM and a boolean for Fixture,
and perform correctness checking in callbacks?
Contributor
Author
There was a problem hiding this comment.
Done — no more string collection. The manager sets an in_measurement_window bool in Start()/Stop(), and the Setup/Teardown callbacks EXPECT it to be false directly, plus *_ran bools so the test fails loudly instead of passing vacuously if a path never executes.
devtejasx
force-pushed
the
fix-memory-manager-measurement-window
branch
from
611bbef to
e083173
Compare
Address review feedback on google#2247: replace the assert-based memory_manager_ordering_test with a gtest that tracks the Start/Stop measurement window and the fixture callbacks with booleans and checks the invariant inside the callbacks, instead of collecting a string log of lifecycle events.
devtejasx
force-pushed
the
fix-memory-manager-measurement-window
branch
from
e083173 to
3a4023d
Compare
dmah42
reviewed
Jul 16, 2026
| MemoryManager::Result BenchmarkRunner::RunMemoryManager( | ||
| IterationCount memory_iterations) { | ||
| memory_manager->Start(); | ||
| // Set up the thread manager and run the user's Setup() outside the |
Member
There was a problem hiding this comment.
not sure what this comment is adding to the code.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RunMemoryManager called memory_manager->Start() before allocating the internal ThreadManager and running the benchmark's Setup(), and called Stop() after resetting the ThreadManager and running Teardown(). As a result the MemoryManager attributed the library's own bookkeeping allocation and any per-benchmark fixture allocations to the benchmark, so the reported allocation count could never be accurate.
Setup()/Teardown() already run outside the timed region; make memory measurement consistent by bracketing only RunInThread with Start()/ Stop(), and move the ThreadManager creation and Setup()/Teardown() outside that window.
Add memory_manager_ordering_test, which registers a MemoryManager and asserts no Setup/Teardown occurs between Start() and Stop(). It fails on the previous ordering and passes with this change.