From 26421c3ca8fbc11cd80395357ffeca7559b4f36d Mon Sep 17 00:00:00 2001
From: Danilo Piparo <danilo.piparo@cern.ch>
Date: Mon, 12 Nov 2018 16:37:49 +0100
Subject: [PATCH] [DF] Remove measures to cope with task interleaving in
RSLotStack
---
tree/dataframe/inc/ROOT/RDF/RSlotStack.hxx | 12 +---
tree/dataframe/src/RSlotStack.cxx | 72 ++++------------------
tree/dataframe/test/dataframe_nodes.cxx | 6 +-
3 files changed, 17 insertions(+), 73 deletions(-)
diff --git a/tree/dataframe/inc/ROOT/RDF/RSlotStack.hxx b/tree/dataframe/inc/ROOT/RDF/RSlotStack.hxx
index 18a68fc8b0a..f4956857758 100644
--- a/tree/dataframe/inc/ROOT/RDF/RSlotStack.hxx
+++ b/tree/dataframe/inc/ROOT/RDF/RSlotStack.hxx
@@ -13,9 +13,7 @@
#include "ROOT/TRWSpinLock.hxx"
-#include <map>
-#include <thread>
-#include <vector>
+#include <stack>
namespace ROOT {
namespace Internal {
@@ -27,10 +25,8 @@ namespace RDF {
/// fixed at construction time and no blocking is foreseen.
class RSlotStack {
private:
- unsigned int &GetCount();
- unsigned int &GetIndex();
- unsigned int fCursor;
- std::vector<unsigned int> fBuf;
+ const unsigned int fSize;
+ std::stack<unsigned int> fStack;
ROOT::TRWSpinLock fRWLock;
public:
@@ -38,8 +34,6 @@ public:
RSlotStack(unsigned int size);
void ReturnSlot(unsigned int slotNumber);
unsigned int GetSlot();
- std::map<std::thread::id, unsigned int> fCountMap;
- std::map<std::thread::id, unsigned int> fIndexMap;
};
} // ns RDF
} // ns Internal
diff --git a/tree/dataframe/src/RSlotStack.cxx b/tree/dataframe/src/RSlotStack.cxx
index 3606e43ad3a..6c78732fa11 100644
--- a/tree/dataframe/src/RSlotStack.cxx
+++ b/tree/dataframe/src/RSlotStack.cxx
@@ -8,77 +8,27 @@
* For the list of contributors see $ROOTSYS/README/CREDITS. *
*************************************************************************/
+#include <ROOT/TSeq.hxx>
#include <ROOT/RDF/RSlotStack.hxx>
#include <TError.h> // R__ASSERT
-#include <limits>
-#include <numeric>
-
-ROOT::Internal::RDF::RSlotStack::RSlotStack(unsigned int size) : fCursor(size), fBuf(size)
-{
- std::iota(fBuf.begin(), fBuf.end(), 0U);
-}
-
-unsigned int &ROOT::Internal::RDF::RSlotStack::GetCount()
-{
- const auto tid = std::this_thread::get_id();
- {
- ROOT::TRWSpinLockReadGuard rg(fRWLock);
- auto it = fCountMap.find(tid);
- if (fCountMap.end() != it)
- return it->second;
- }
-
- {
- ROOT::TRWSpinLockWriteGuard rg(fRWLock);
- return (fCountMap[tid] = 0U);
- }
-}
-unsigned int &ROOT::Internal::RDF::RSlotStack::GetIndex()
+ROOT::Internal::RDF::RSlotStack::RSlotStack(unsigned int size) : fSize(size)
{
- const auto tid = std::this_thread::get_id();
-
- {
- ROOT::TRWSpinLockReadGuard rg(fRWLock);
- auto it = fIndexMap.find(tid);
- if (fIndexMap.end() != it)
- return it->second;
- }
-
- {
- ROOT::TRWSpinLockWriteGuard rg(fRWLock);
- return (fIndexMap[tid] = std::numeric_limits<unsigned int>::max());
- }
+ for (auto i : ROOT::TSeqU(size)) fStack.push(i);
}
-void ROOT::Internal::RDF::RSlotStack::ReturnSlot(unsigned int slotNumber)
+void ROOT::Internal::RDF::RSlotStack::ReturnSlot(unsigned int slot)
{
- auto &index = GetIndex();
- auto &count = GetCount();
- R__ASSERT(count > 0U && "RSlotStack has a reference count relative to an index which will become negative.");
- count--;
- if (0U == count) {
- index = std::numeric_limits<unsigned int>::max();
- ROOT::TRWSpinLockWriteGuard guard(fRWLock);
- fBuf[fCursor++] = slotNumber;
- R__ASSERT(fCursor <= fBuf.size() &&
- "RSlotStack assumes that at most a fixed number of values can be present in the "
- "stack. fCursor is greater than the size of the internal buffer. This violates "
- "such assumption.");
- }
+ ROOT::TRWSpinLockWriteGuard guard(fRWLock);
+ R__ASSERT(fStack.size() < fSize && "Trying to put back a slot to a full stack!");
+ fStack.push(slot);
}
unsigned int ROOT::Internal::RDF::RSlotStack::GetSlot()
{
- auto &index = GetIndex();
- auto &count = GetCount();
- count++;
- if (std::numeric_limits<unsigned int>::max() != index)
- return index;
ROOT::TRWSpinLockWriteGuard guard(fRWLock);
- R__ASSERT(fCursor > 0 &&
- "RSlotStack assumes that a value can be always obtained. In this case fCursor is <=0 and this "
- "violates such assumption.");
- index = fBuf[--fCursor];
- return index;
+ R__ASSERT(!fStack.empty() && "Trying to pop a slot from an empty stack!");
+ const auto slot = fStack.top();
+ fStack.pop();
+ return slot;
}
diff --git a/tree/dataframe/test/dataframe_nodes.cxx b/tree/dataframe/test/dataframe_nodes.cxx
index 9afdcd79135..a3961065be7 100644
--- a/tree/dataframe/test/dataframe_nodes.cxx
+++ b/tree/dataframe/test/dataframe_nodes.cxx
@@ -11,7 +11,7 @@ TEST(RDataFrameNodes, RSlotStackCheckSameThreadSameSlot)
{
unsigned int n(7);
ROOT::Internal::RDF::RSlotStack s(n);
- EXPECT_EQ(s.GetSlot(), s.GetSlot());
+ EXPECT_EQ(s.GetSlot()-1, s.GetSlot());
}
#ifndef NDEBUG
@@ -32,7 +32,7 @@ TEST(RDataFrameNodes, RSlotStackGetOneTooMuch)
t.join();
};
- EXPECT_DEATH(theTest(), "RSlotStack assumes that a value can be always obtained.");
+ EXPECT_DEATH(theTest(), "Trying to pop a slot from an empty stack!");
}
TEST(RDataFrameNodes, RSlotStackPutBackTooMany)
@@ -42,7 +42,7 @@ TEST(RDataFrameNodes, RSlotStackPutBackTooMany)
s.ReturnSlot(0);
};
- EXPECT_DEATH(theTest(), "RSlotStack has a reference count relative to an index which will become negative");
+ EXPECT_DEATH(theTest(), "Trying to put back a slot to a full stack!");
}
#endif
--
GitLab