Remove double locking in prepareForWrite

[jmcarcell]

BEGINRELEASENOTES

• Remove double locking in prepareForWrite; acquire the lock only once

ENDRELEASENOTES

I think this double locking is unnecessary. The first time the mutex is locked only a check is performed that is also done anyway the second time, so locking once should be enough. In addition there is a test specifically for this that passes:

podio/tests/unittests/unittest.cpp

Line 345 in aa5927b

TEST_CASE("thread-safe prepareForWrite", "[basics][multithread]") {

Added originally in #295

[m-fila]

I was looking at this some time ago and was a bit confused. From what I understood the idea was to have a faster look-up and possibly slower modification, right?

• I agree that the first lock in "double locking" seems to be redundant.
• m_isPrepared is protected with mutex only in prepareForWrite yet there are other methods reading and writing to it.
• Considering the atomic, the move constructor could construct from value loaded from moved object (m_isPrepared(other.m_isPrepared.load())). I'm not sure if this would matter for our use-case but such constructor does two operations and itself isn't atomic. I think the bigger issue is that the construct can't be default anymore
• There is also std::shared_mutex that has shared access (for reading) and exclusive access (for writing), although I'm not sure if it's worth it, perhaps having just a single lock with simple mutex is enough?

There is another double locking for link collections:

podio/include/podio/detail/LinkCollectionImpl.h

Lines 263 to 282 in aa5927b

	void prepareForWrite() const override {
	// TODO: Replace this double locking pattern with an atomic and only one
	// lock. Problem: std::atomic is not default movable
	{
	std::lock_guard lock{*m_storageMtx};
	// If the collection has been prepared already there is nothing to do
	if (m_isPrepared) {
	return;
	}
	}
	std::lock_guard lock{*m_storageMtx};
	// by the time we acquire the lock another thread might have already
	// succeeded, so we need to check again now
	if (m_isPrepared) {
	return;
	}
	m_storage.prepareForWrite(m_isSubsetColl);
	m_isPrepared = true;
	}

[tmadlener]

It has been some time since I wrote this, so I am also no longer entirely sure whether the double locking is necessary. At the time I thought it is.

prepareForWrite needs to happen before a collection is written and it can only happen once per collection. The double locking is there at the moment, because in principle two writers could write the same Frame, and so it's possible that this happens on separate threads and it's not clear which thread will get to prepare a given collection first. A collection that is read from file is automatically prepared, because it reads the buffers from file and keeps them around.

So all of this is less of an optimization, but rather making sure that prepareForWrite is called exactly once. For that I went with a double locking, because if thread 1 does the preparation while thread 2 reads m_isPrepared, thread 2 should not to the preparation again, but rather check again, and see that thread 1 already went through.

[m-fila]

So all of this is less of an optimization, but rather making sure that prepareForWrite is called exactly once. For that I went with a double locking, because if thread 1 does the preparation while thread 2 reads m_isPrepared, thread 2 should not to the preparation again, but rather check again, and see that thread 1 already went through.

Then single lock is enough, thread 2 can't even read m_isPrepared as it's behind mutex that was already acquired by thread 1, so it has to wait for thread 1 to prepare the collection and unlock. The guarantees are the same with single and double locking written as is. (assuming both threads want to write the collection, if one of them writes and the other clears, then it's a problem for both single and double lock)

[jmcarcell]

I also removed it in LinkCollectionImpl.h and agree with what @m-fila said.

[tmadlener]

Thanks :)