rmp: use std::atomic for Blif::call_id_ static data member

[cc10512]

Protect concurrent access to the Blif::call_id_ using an atomic data type: it is cheaper than a lock and for a machine word it is sufficent.

Resolves rmp in #5981.

[maliberty]

@QuantamHD I see no reason for this to be static at all. Can we just make it a class member?

[maliberty]

Part of the goal in #5981 is to allow multiple instances of OpenRoad to operate on different designs independently. This would solve thread-safety but not isolation (you would get different values of call_id_ depending on what each instance is doing).

[QuantamHD]

So I looked through the code, and this is very similar to what I'm doing in my rewrite. The std::atomic change is good, but we need to add a new class variable to the Blif class that stores the current value of the std::atomic.

[maliberty]

@QuantamHD it if it is a class var there is no need for atomic or static.

[QuantamHD]

It's supposed to provide a unique id (for instance naming purposes) across the whole application even if threads are creating multiple blif objects.

call_id_instance_ = Blif::call_id.fetch_add(1);

[maliberty]

It's supposed to provide a unique id (for instance naming purposes) across the whole application even if threads are creating multiple blif objects.

call_id_instance_ = Blif::call_id.fetch_add(1);

The names will be unstable and if abc has any dependency on them then the results will be unstable. It seems better to put the counter inside the thread than share it regardless of atomic/lock.

[QuantamHD]

So making it thread_local? You need some way to persist the id across constructors.

[QuantamHD]

Maybe there's a bit of confusion here. The point of this call_id is to give unique object names to things coming back into OpenROAD from ABC

[maliberty]

Are different threads adding to the same db? If so, then thread local won't ensure uniqueness (and requires a lock). If not then thread local would work.

[cc10512]

Thank you both for your comments.
First, let me explain how I understand this code, what the PR does, and then we can be more explicit about what's required.

Blif::call_id_ is a static data member, therefore a global not tied to any instance. It essentially counts instances because it is only incremented in the constructor (blif.cpp:80). It is read a number of times in Blif::readBlif to create unique names. AFAICT, the constructor is also called only once in rmp.i:86.

By making it atomic, we ensure it is correctly incremented as a global counter (operator ++ on std::atomic does a fetch_and_add). It will continue to count instances, and indeed, with the current way it is used, every read will give you the latest value written, not identify a particular instance.

It will also only count instances in the same process; that is, as long as we only spawn threads, we'll get the global counter behavior (and it is thread safe). If we fork a different process, that process will start counting from the current value, and the two processes will have overlapping sequences.

What do we want?

1. If we want global counting to generate unique names, we'll need to increment call_id_ when we create the nets in Blif::readBlif. The atomic static data member will work as long as we do not fork processes.
2. If we want to count instances in a multithreaded environment, then, yes, @QuantamHD's suggestion of saving the instance counter as a data member and using that to create the names will generate unique names for each instance.
3. If we want a global counter across multiple processes, then we need a different protocol -- one that allows information exchange across processes. In principle that can be done through common storage or by using Message Passing Interface (MPI) like naming rank_id:instance_id.

Let me know which one of these is the intended design.

[QuantamHD]

Are different threads adding to the same db? If so, then thread local won't ensure uniqueness (and requires a lock). If not then thread local would work.

No I think this is meant to be single threaded. @maliberty I think option 2 of @cc10512's should be what we move forward with. Do you concur?

[maliberty]

Does option 2 mean "So making it thread_local? You need some way to persist the id across constructors."? If so, yes.

[cc10512]

@maliberty option 2 means that you make it "instance local". You have a global counter that's thread safe, and if you save that in your instance, then each instance will have a unique value.

What option 2 will not do is guarantee thread safety for instances -- that is, if you share an instance pointer across threads, then we'll have to look at the thread safety for all the other methods and data members. However, the since this new instance_id data member will be currently assigned once (in the constructor, therefore before one can share the pointer) and then only read, then all threads "using" this instance will see the same value.

[cc10512]

Based on our discussion until now, I believe option 2 is what we want. I pushed a commit to that effect so that @maliberty can see the code and decide.

[cc10512]

Here is a code example that shows how this works in a multithreaded environment:

#include <atomic>
#include <cassert>
#include <future>
#include <iostream>
#include <thread>
#include <vector>

class A {
public:
  static int              _non_safe_counter;
  static std::atomic<int> _safe_counter;
  int                     _instance_id;

  A() {
    _non_safe_counter++;
    _instance_id = ++_safe_counter;
  }
};

int A::_non_safe_counter = 0;
std::atomic<int> A::_safe_counter{0};
constexpr int ITERS = 10000;

void worker(std::promise<int> promise)
{
  A *a;
  for(int i = 0; i < ITERS; i++)
    a = new A();

  promise.set_value(a->_instance_id);
}

int main()
{
  constexpr int N = 10;
  std::vector<std::thread> pool;
  std::vector<std::promise<int>> results;
  std::vector<std::future<int>> futures;

  // setup the return results as futures based on promises
  for (int n = 0; n < N; n++) {
    std::promise<int> *default_promise = new std::promise<int>();
    futures.push_back(default_promise->get_future());
    results.push_back(std::move(*default_promise));
  }

  // create the treads; they start running as soon as they are constructed
  for (int n = 0; n < N; n++)
    pool.push_back(std::thread(worker, std::move(results[n])));

  // join the thread so that we ensure that all the work is finished
  for (int n = 0; n < N; n++)
    pool[n].join();

  // create one new instance: its id should be N * ITERS + 1
  A *a = new A();
  for (int n = 0; n < N; n++) {
    auto ai = futures[n].get();
    // show a proper interleaving of threads; these should be random numbers
    // close to max (N * ITERS)
    std::cout << "Thread " << n << ": instance id is: " << ai << "\n";
  }
  std::cout << "Global: atomic counter is " << a->_safe_counter << "\n\t"
            << "non-atomic counter is " << a->_non_safe_counter << "\n\t"
            << "instance_id is " << a->_instance_id << "\n";

  assert(a->_instance_id == N * ITERS + 1);
  return 0;
}

Compile with clang++ --std=c++20 instances.cc

The output of this program is:

Thread 0: instance id is: 61030
Thread 1: instance id is: 72861
Thread 2: instance id is: 92077
Thread 3: instance id is: 81543
Thread 4: instance id is: 98120
Thread 5: instance id is: 82180
Thread 6: instance id is: 95331
Thread 7: instance id is: 77188
Thread 8: instance id is: 99926
Thread 9: instance id is: 100000
Global: atomic counter is 100001
	non-atomic counter is 41803
	instance_id is 100001

I hope this convinces you that the handling of instances is thread safe.

[maliberty]

Equivalent to instance_id_ = instance_counter_++;

[maliberty]

I am confused about the goal here. @QuantamHD is rewritting this code not use blif at all but to directly call abc. The current code doesn't use threads. It seems we are expending effort on condemned code.

When we have multiple OpenRoad objects in a single process then we don't want any sharing through instance_counter_ even if atomic. It makes the object names non-deterministic and anything that depends on them will be such. @QuantamHD suggested have name generation happen in odb which is reasonable and would kill this variable altogether (replacing it with a non-static in the db).

[QuantamHD]

I hadn't had a chance to respond, but I realized you were right @maliberty. The end state we want is to have hermetic objects in the same thread.

Design x1;
Design x2;

x1.doStuff();
x2.doStuff();

x1.writeVerilog();
x2.writeVerilog();

If we leave the static around the output of x1 and x2 will depend on the order in which they're run. We should in-fact move the counter to this class which has a 1-1 correspondence with Design https://github.com/The-OpenROAD-Project/OpenROAD/blob/master/src/rmp/include/rmp/Restructure.h as @maliberty.

Sorry for the confusion @cc10512, we should in-fact remove the static and move it to Restructure.h and pipe it to this location. Otherwise we'll create non-hermetic outputs.

It seems we are expending effort on condemned code.

I need this functionality for my rewrite, so it's not wasted effort @maliberty

[maliberty]

Ok so it sounds like this PR is moot and you'll approach solving it as you suggest.