[BUG] CPU hang on multithreaded benchmarks overriding the memory manager

[jlecavelier]

Describe the bug
Consider a multithreaded benchmark scenario which overrides the memory manager.
In this scenario we want all the benchmark threads to synchronize at a given point in time and we use benchmark::State::threads() to fetch the number of threads that need to be synced.
This leads to a CPU hang because when the memory manager is overriden, BenchmarkRunner::DoOneRepetition() runs a final pass to gather memory statistics on a single thread while recycling a fixture that was designed for multithreaded execution.

Here is a minimal repro scenario:

#include <stdio.h>
#include <benchmark/benchmark.h>

#include <condition_variable>
#include <mutex>
#include <thread>

// Set this to 0 to remove the hang
#define MEMORY_MANAGER_OVERRIDE 1

/*
Minimal repro:
When performing a benchmark and overriding the memory manager, state.threads() becomes unreliable;
This happens because a memory benchmark test will be ran on a single thread, recycling a fixture that was created for a multi-threaded scenario
*/

using cscoped_lock = std::unique_lock<std::mutex>;

class memory_manager final : public benchmark::MemoryManager {
public:
	memory_manager() = default;
	void Start() override final {};
	void Stop(Result&) override final {};
};

class benchmark_fixture : public benchmark::Fixture {
	std::mutex m_mutex;
	std::condition_variable m_cv;
	uint32_t m_num_threads = 0;

private:
	void SetUp(const benchmark::State& _state) override final;	// Waits on every thread to enter before calling to OnBenchmarkSetup
	void TearDown(const benchmark::State& _state) override final;	// Waits on every thread to enter before calling to OnBenchmarkTearDown

private:
	void OnBenchmarkSetup() {};
	void OnBenchmarkTearDown() {};

public:
	virtual ~benchmark_fixture() = default;
};

void benchmark_fixture::SetUp(const benchmark::State& _state) {
	const uint32_t thread_count = _state.threads();

	cscoped_lock lock(m_mutex);
	++m_num_threads;

	const bool startup = m_num_threads == thread_count;
	if (startup) {
		OnBenchmarkSetup();	// The last thread to enter will perform the benchmark setup
		m_cv.notify_all();
	} else {
		m_cv.wait(lock);
	}
}

void benchmark_fixture::TearDown(const benchmark::State& _state) {
	const uint32_t thread_count = _state.threads();

	cscoped_lock lock(m_mutex);
	--m_num_threads;
	const bool teardown = m_num_threads == 0;
	if (teardown) {
		OnBenchmarkTearDown(); // The last thread to enter will perform the benchmark teardown
		m_cv.notify_all();
	} else {
		m_cv.wait(lock);
	}
}

BENCHMARK_DEFINE_F(benchmark_fixture, run)
(benchmark::State& _state) {
	constexpr uint32_t wait_time[] = { 16, 14, 12, 10, 8, 6, 4, 2, 0 };

	// Wait an arbitrary period of time to simulate work
	for (auto _ : _state) {
		const uint32_t thread_index = _state.thread_index();
		const uint32_t wait_time_ms = wait_time[thread_index / 2];
		std::this_thread::sleep_for(std::chrono::milliseconds(wait_time_ms));
	}
}
BENCHMARK_REGISTER_F(benchmark_fixture, run)->Iterations(4)->ThreadRange(1, 16);

int main(int argc, char** argv) {
	char arg0_default[] = "benchmark";
	char* args_default = arg0_default;
	if (!argv) {
		argc = 1;
		argv = &args_default;
	}
	::benchmark::Initialize(&argc, argv);
	if (::benchmark::ReportUnrecognizedArguments(argc, argv)) {
		return 1;
	}

#if MEMORY_MANAGER_OVERRIDE
	memory_manager mem_mgr;
	::benchmark::RegisterMemoryManager(&mem_mgr);
#endif

	::benchmark::RunSpecifiedBenchmarks();
	::benchmark::Shutdown();
	return 0;
}

Why are memory statistics gathered in a multithreaded scenario not following the same path as regular fixture runs?
Why are they recycling a fixture that was built to run in a multithreaded fashion to perform a single threaded test?

[dmah42]

you shouldn't need a fixture for this i believe, as the section before the benchmarked loop is only run on thread 0: https://google.github.io/benchmark/user_guide.html#multithreaded-benchmarks "it is guaranteed that none of the threads will start until all have reached the start of the benchmark loop, and all will have finished before any thread exits the benchmark loop".

i think you can hugely simplify your code.

instead of answering your questions about why the library works the way that it does, what exactly are you trying to do?

[jlecavelier]

Yes, you are correct, the code to achieve this very specific behavior could be much more simple. This code's purpose is to provide a minimal repro of an API issue. This issue will be encountered in any scenario where you might need to rely on benchmark::State::threads to perform a sync of all of your threads at any point in time in you benchmark. Let me provide more context with a different hypothetical scenario.
Imagine that I am making a rendering framework and I want to profile some tasks that happen before and after I kick off a frame ( for instance: recording command buffers, creating view descriptors, etc, etc. ). But I do not want to profile the frame kick-off itself.
To do that I'm going to perform the work I want to profile and measure it and then I will synchronize my threads and pause the timing of my test while my frame is being processed and until I want profiling to resume.
In this scenario I will encounter the same issue as in the minimal example because benchmark::State::threads is simply not behaving as described in the documentation.

Now, regardless of any scenario, wouldn't you agree that benchmark::State::threads not behaving as documented and returning an incorrect value (because that is what is is doing) is a flaw here?
Wouldn't you agree that it is reasonable for someone who registers a benchmark to run in a multithreaded fashion, explicitly overriding the memory manager, to expect gathering of memory statistics happening as they set it up, in a multi threaded fashion, and not in serial?

Thank you for your time.

[dmah42]

why would multithreading affect memory measurement?

[jlecavelier]

Because you might want to employ different allocation strategies when you encounter contention, for example. This is not the topic of this thread however.
Let's please not veer off the very simple issue that this ticket was created for and for which I not only provided a basic repro scenario but also formulated a very simple question that you don't seem to want to address; are you able, by any chance, to answer the very simple, specific question I formulated and for which I created this ticket:
Wouldn't you agree that benchmark::State::threads not behaving as documented and returning an incorrect value when the memory manager is overridden is a flaw here?
I am sure you have grasped the substance of the issue at hand here and if you think it's not worth spending time solving it then let's please be honest about it, close this ticket and move on.
In any case this thread might be useful for people running into the same issue of the API not acting as documented and they will know why & that this is also intended behavior.
Thank you.