6
\$\begingroup\$

Because I have been writing more multithreaded C++14 code lately, I've developed some tools that help me make sure that my threads are all working as intended. In the real code, it's often difficult to detect race conditions or unintended sequence dependencies among threads, so I have found that temporarily substituting a work simulator (worksim in the code) for the real thread code, leaving any other locks, mutexes, etc. in place, allows me to gain some insight as to how the various threads might run.

There are three main pieces in the code below. The first is the Logger class which is more or less a mutex-protected interface to std::cout. It takes a std::stringstream & as input, emits the contents of it to std::cout and the clears the input stringstream.

The second piece is the worksim itself which employs a binomial random number distribution to generate delays that are typically around 25ms, emitting status to the passed Logger object until the stop flag is set. I deliberately do not want to use a conditional variable here because I want to simulate the work that the thread would do.

The third piece is a simple main showing how these are used. It creates 10 threads, waits five seconds and then shuts everything down.

I'm interested in comments on the worksim portion in particular. Specifically,

  1. is the intent clear?
  2. is the use of a static number generator OK here?
  3. is there a better way to handle the logging?

threadplay.cpp

#include <iostream>
#include <atomic>
#include <mutex>
#include <thread>
#include <random>
#include <chrono>
#include <vector>
#include <sstream>

class Logger {
public:
    Logger &operator<<(std::stringstream &ss) {
        std::lock_guard<std::mutex> lock(m);
        std::cout << ss.str();
        ss.str(std::string{});
        return *this;
    }
private:
    std::mutex m;
};

void worksim(Logger &mylog, std::atomic<bool> &stop, int n) {
    using namespace std::chrono_literals;
    static std::mt19937 gen(std::random_device{}());
    std::binomial_distribution<int> dist(50, 0.5);
    std::stringstream ss;
    ss << "Checking " << n << "\n";
    mylog << ss;
    for (auto delay = dist(gen); !stop; delay = dist(gen)) {
        ss << "Thread " << n << " sleeping for " << delay << "ms\n";
        mylog << ss;
        std::this_thread::sleep_for(1ms * delay);
    }
    ss << "Leaving " << n << "\n";
    mylog << ss;
}

int main()
{
    using namespace std::chrono_literals;
    std::atomic<bool> stop{false};
    std::vector<std::thread> th;
    Logger mylog;
    for (int i=0; i<10; ++i) {
        th.push_back(std::thread(worksim, std::ref(mylog), std::ref(stop), i));
    }
    std::this_thread::sleep_for(5s);
    stop = true;
    for (std::thread &thread : th) {
        thread.join();
    }
}
\$\endgroup\$
  • \$\begingroup\$ The use of a shared logger with the mutex locking may severely influence your threads' runtime behavior. Is that supposed for debugging purposes only? \$\endgroup\$ – πάντα ῥεῖ Dec 31 '16 at 13:37
  • \$\begingroup\$ Yes, it's for debugging and experimental purposes only. Ordinarily, each thread would have its own log which would eliminate the need for a mutex. \$\endgroup\$ – Edward Dec 31 '16 at 13:43
  • \$\begingroup\$ The canonical way to use pseudo-random number generators in multi-thread environments in to make them thread_local. That way, you don't have to guard them, but you keep the thread safety. If I'm not mistaken, it's even ok to make the distribution itself thread_local. \$\endgroup\$ – Morwenn Jan 2 '17 at 23:18
  • \$\begingroup\$ In a somewhat near future, you could use std::osyncstream to guard std::cout in your logger, instead of manually locking it :) \$\endgroup\$ – Morwenn Jan 2 '17 at 23:24
  • \$\begingroup\$ Also please emplace_back your threads, it will be way more sexy :D \$\endgroup\$ – Morwenn Jan 2 '17 at 23:26
1
\$\begingroup\$

is the intent clear?

I think the intent is obvious.

is the use of a static number generator OK here?

I don't think so. I don't believe that the random number generator is thread safe. So to accesses it from multiple threads (as you are doing) you should lock accesses with a mutex.

is there a better way to handle the logging?

I don't like having to create a std::stringstream seprately and then pass that manually to the logger. I would wrap this up a bit:

class Logger;

class LogStream
{
    Logger&             parent;
    std::stringstream   ss;
    public:
        LogStream(Logger& parent)
            : parent(parent)
        {}
        ~LogStream();
        template<typename T>
        LogStream& operator<<(T const& t) {
            ss << t;
            return *this;
        }
};
class Logger
{
    public:
        Logger(std::ostream& log)
            : log(log)
        {}
        void print(std::stringstream &ss) {
            std::lock_guard<std::mutex> lock(m);
            log << ss.str();
        }
    private:
        std::ostream&   log;
        std::mutex      m;
};
LogStream::~LogStream()
{
    parent.print(ss);
}

Then your usage changes:

std::stringstream ss;
ss << "Checking " << n << "\n";
mylog << ss;

// To 

LogStream(mylog) << "Checking " << n << "\n";
\$\endgroup\$
  • \$\begingroup\$ "I think the intent is obvious." Can you repeat it in your own words please? \$\endgroup\$ – πάντα ῥεῖ Jan 2 '17 at 17:27
2
\$\begingroup\$

1. Is the intent clear?

Not completely for me. As you're saying

for the real thread code, leaving any other locks, mutexes, etc. in place, allows me to gain some insight as to how the various threads might run.

the closest intent I can grasp is, you want to eliminate certain threads by replacing them with worksim threads, to isolate other threads that might be the source of race conditions, dead locks or other multiple threading related problems in the to be tested code.

I'm not a 100% sure if these simplified threads may be really useful for diagnosis of the above mentioned problems. Especially as they won't affect other threads, other than with a random delay and don't touch those synchronization mechanisms left in place. To some degree may be.

2. Is the use of a static number generator OK here?

The instantiation of static local variables will be thread safe. Execution of operations like dist(gen)) will not guarantee thread safety.

The reference documentation about std::binomial_distribution::operator() doesn't tell anything about thread safety, and because gen is a shared resource between threads I would doubt about appropriate results.

3. is there a better way to handle the logging?

The standard I/O output can be fairly slow, and as I mentioned in my comment "The use of a shared logger with the mutex locking may severely influence your threads' runtime behavior".

A generally better approach might be to use the shortest possible lock to pass the log message to the logger (e.g. using a protected queue of std::unique_ptr's), and let the Logger class output these from another background thread (that's what we're using in our production systems).

\$\endgroup\$
  • \$\begingroup\$ It is a common misconception that Standard I/O can be fairly slow. Though it is slightly slower (because of the extra calls for formatting for local) than the C routines not significantly enough to make timing difference (unless there is a huge amount of output). The main reason for this misconception is that people forget to unlink the C++ streams from the C streams std::ios_base::sync_with_stdio and that std::cin and std::cout are tied together std::basic_ios::tie. \$\endgroup\$ – Martin York Jan 1 '17 at 21:34
  • \$\begingroup\$ Remove these restraints on the C++ I/O streams and they become significantly faster and are comparable to C I/O. But yes I/O operations in general are slow in comparison to nearly everything else. \$\endgroup\$ – Martin York Jan 1 '17 at 21:35
  • 1
    \$\begingroup\$ @LokiAstari I'm not talking about (or having) that misconception. I'm talking about the concurrency lock appearing over the whole time needed to execute the std::cout statement, and it's interference with the overall timing of the other threads. \$\endgroup\$ – πάντα ῥεῖ Jan 2 '17 at 14:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.