stan-dev / math

The Stan Math Library is a C++ template library for automatic differentiation of any order using forward, reverse, and mixed modes. It includes a range of built-in functions for probabilistic modeling, linear algebra, and equation solving.
https://mc-stan.org
BSD 3-Clause "New" or "Revised" License
723 stars 183 forks source link

Profiling system is not thread safe #3062

Closed WardBrian closed 1 month ago

WardBrian commented 1 month ago

Description

The profiling system has some data races which lead to occasional crashes in CI (and likely bad behavior in the wild).

The most recent failure can be seen: https://jenkins.flatironinstitute.org/blue/organizations/jenkins/Stan%2FMath/detail/PR-3061/1/pipeline/378

test/unit/math/rev/core/profiling_threading_test --gtest_output="xml:test/unit/math/rev/core/profiling_threading_test.xml"
Running main() from lib/benchmark_1.5.1/googletest/googletest/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from Profiling
[ RUN      ] Profiling.profile_threading
Segmentation fault (core dumped)
test/unit/math/rev/core/profiling_threading_test --gtest_output="xml:test/unit/math/rev/core/profiling_threading_test.xml" failed
exit now (05/04/24 13:26:40 EDT)
139

Example

I'm pretty sure we're just straight up doing things that aren't allowed with std::map. -fsanitize=thread complains about it, but also complains about a bunch of other stuff from tbb.

A more minimal example of the kind of thing the profiler is doing:

#include <map>
#include <thread>
#include <chrono>
#include <vector>
#include <string>
#include <iostream>

using profile_key = std::pair<std::string, std::thread::id>;
using profile_map = std::map<profile_key, int>;

class profile {
  profile_key key_;
  int* profile_;

 public:
  profile(std::string name, profile_map& profiles)
      : key_({name, std::this_thread::get_id()}) {
    profile_map::iterator p = profiles.find(key_);
    if (p == profiles.end()) {
      profiles[key_] = 0;
    }
    profile_ = &profiles[key_];
  }
  // ~profile() { profile_ += 1; }
};

profile_map threading_map;

void foo() {
    profile p("foo", threading_map);
    std::cout << "foo" << std::endl;
    // std::this_thread::sleep_for(std::chrono::seconds(1));
}

int main(int argc, char** argv) {
    std::vector<std::thread> threads;
    for (int i = 0; i < 20; i++) {
        threads.push_back(std::thread(foo));
    }
    for (auto& t : threads) {
        t.join();
    }
    return 0;

}

If you build g++ -g -O3 min.cpp -o min -pthread

and run this program a ton of times

while ./min; do :; done

it will segfault pretty quickly.

I think we are assuming that because the thread_id is in the key, we are safe, but I see no place where std::map is guaranteed to be safe for concurrent writes, even if the keys are not shared.

Expected Output

Tests pass and don't have data races.

Current Version:

v4.8.1

SteveBronder commented 1 month ago

Yes true we should be using the tbb::concurrent_map