Skip to content

Commit 552ccdd

Browse files
TheMostDiligentTheMostDiligent
committed
GPUUploadManagerImpl::Page: add Writer helper for update handling
1 parent 8311a9b commit 552ccdd

3 files changed

Lines changed: 160 additions & 56 deletions

File tree

‎Graphics/GraphicsTools/include/GPUUploadManagerImpl.hpp‎

Lines changed: 49 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -69,17 +69,49 @@ class GPUUploadManagerImpl final : public ObjectBase<IGPUUploadManager>
6969
IDeviceContext* pContext,
7070
Uint32 Size);
7171

72-
// Tries to begin writing to the page. Returns false if the page is sealed for new writes.
73-
// Otherswise, increments the number of active writers and returns true.
74-
bool TryBeginWriting();
75-
7672
enum class WritingStatus
7773
{
7874
NotSealed,
7975
NotLastWriter,
8076
LastWriterSealed
8177
};
82-
WritingStatus EndWriting();
78+
79+
class Writer
80+
{
81+
public:
82+
operator bool() const { return m_pPage != nullptr; }
83+
84+
// clang-format off
85+
Writer (const Writer&) = delete;
86+
Writer& operator=(const Writer&) = delete;
87+
Writer (Writer&& Other) noexcept;
88+
Writer& operator=(Writer&& Other) noexcept;
89+
// clang-format on
90+
91+
bool ScheduleBufferUpdate(IBuffer* pDstBuffer,
92+
Uint32 DstOffset,
93+
Uint32 NumBytes,
94+
const void* pSrcData,
95+
GPUUploadExecutedCallbackType Callback,
96+
void* pCallbackData);
97+
98+
WritingStatus EndWriting();
99+
100+
~Writer();
101+
102+
private:
103+
friend Page;
104+
explicit Writer(Page* pPage) noexcept :
105+
m_pPage{pPage}
106+
{}
107+
108+
private:
109+
Page* m_pPage = nullptr;
110+
};
111+
112+
// Tries to begin writing to the page. Returns a valid Writer object if the
113+
// page is not sealed for new writes, and an empty Writer otherwise.
114+
Writer TryBeginWriting();
83115

84116
enum class SealStatus
85117
{
@@ -98,15 +130,6 @@ class GPUUploadManagerImpl final : public ObjectBase<IGPUUploadManager>
98130
// Seals the page for new writes and returns the sealing status.
99131
SealStatus TrySeal();
100132

101-
// Schedules a buffer update operation on the page.
102-
// Returns true if the operation was successfully scheduled, and false otherwise.
103-
bool ScheduleBufferUpdate(IBuffer* pDstBuffer,
104-
Uint32 DstOffset,
105-
Uint32 NumBytes,
106-
const void* pSrcData,
107-
GPUUploadExecutedCallbackType Callback,
108-
void* pCallbackData);
109-
110133
void ExecutePendingOps(IDeviceContext* pContext, Uint64 FenceValue);
111134
void Reset(IDeviceContext* pContext);
112135

@@ -126,6 +149,18 @@ class GPUUploadManagerImpl final : public ObjectBase<IGPUUploadManager>
126149
// Returns true if the page is sealed for new writes. This is used for testing and debugging purposes.
127150
bool IsSealed() const { return (m_State.load(std::memory_order_acquire) & SEALED_BIT) != 0; }
128151

152+
private:
153+
// Schedules a buffer update operation on the page.
154+
// Returns true if the operation was successfully scheduled, and false otherwise.
155+
bool ScheduleBufferUpdate(IBuffer* pDstBuffer,
156+
Uint32 DstOffset,
157+
Uint32 NumBytes,
158+
const void* pSrcData,
159+
GPUUploadExecutedCallbackType Callback,
160+
void* pCallbackData);
161+
162+
WritingStatus EndWriting();
163+
129164
private:
130165
const Uint32 m_Size = 0;
131166

‎Graphics/GraphicsTools/src/GPUUploadManagerImpl.cpp‎

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,60 @@
3636
namespace Diligent
3737
{
3838

39+
GPUUploadManagerImpl::Page::Writer::Writer(Writer&& Other) noexcept :
40+
m_pPage{Other.m_pPage}
41+
{
42+
Other.m_pPage = nullptr;
43+
}
44+
45+
GPUUploadManagerImpl::Page::Writer& GPUUploadManagerImpl::Page::Writer::operator=(Writer&& Other) noexcept
46+
{
47+
if (this != &Other)
48+
{
49+
m_pPage = Other.m_pPage;
50+
Other.m_pPage = nullptr;
51+
}
52+
return *this;
53+
}
54+
55+
bool GPUUploadManagerImpl::Page::Writer::ScheduleBufferUpdate(IBuffer* pDstBuffer,
56+
Uint32 DstOffset,
57+
Uint32 NumBytes,
58+
const void* pSrcData,
59+
GPUUploadExecutedCallbackType Callback,
60+
void* pCallbackData)
61+
{
62+
if (m_pPage == nullptr)
63+
{
64+
UNEXPECTED("Attempting to schedule a buffer update with an invalid writer.");
65+
return false;
66+
}
67+
68+
return m_pPage->ScheduleBufferUpdate(pDstBuffer, DstOffset, NumBytes, pSrcData, Callback, pCallbackData);
69+
}
70+
71+
GPUUploadManagerImpl::Page::WritingStatus GPUUploadManagerImpl::Page::Writer::EndWriting()
72+
{
73+
if (m_pPage == nullptr)
74+
{
75+
UNEXPECTED("Attempting to end writing with an invalid writer.");
76+
return WritingStatus::NotSealed;
77+
}
78+
79+
WritingStatus Status = m_pPage->EndWriting();
80+
m_pPage = nullptr;
81+
return Status;
82+
}
83+
84+
GPUUploadManagerImpl::Page::Writer::~Writer()
85+
{
86+
if (m_pPage != nullptr)
87+
{
88+
UNEXPECTED("Writer was not explicitly ended. Ending writing in destructor.");
89+
EndWriting();
90+
}
91+
}
92+
3993
GPUUploadManagerImpl::Page::Page(Uint32 Size) noexcept :
4094
m_Size{Size}
4195
{}
@@ -60,29 +114,29 @@ GPUUploadManagerImpl::Page::Page(IRenderDevice* pDevice,
60114
VERIFY_EXPR(m_pData != nullptr);
61115
}
62116

63-
bool GPUUploadManagerImpl::Page::TryBeginWriting()
117+
GPUUploadManagerImpl::Page::Writer GPUUploadManagerImpl::Page::TryBeginWriting()
64118
{
65119
Uint32 State = m_State.load(std::memory_order_acquire);
66120
for (;;)
67121
{
68122
if (State & SEALED_BIT)
69123
{
70124
// The page is sealed for new writes.
71-
return false;
125+
return Writer{nullptr};
72126
}
73127

74128
if ((State & WRITER_MASK) == WRITER_MASK)
75129
{
76130
// Too many writers.
77131
// This should never happen in practice, but we handle this case for robustness.
78-
return false;
132+
return Writer{nullptr};
79133
}
80134

81135
if (m_State.compare_exchange_weak(
82136
State, // On failure, updated state is written back to State variable.
83137
State + 1,
84138
std::memory_order_acq_rel))
85-
return true;
139+
return Writer{this};
86140
}
87141
}
88142

‎Tests/DiligentCoreTest/src/GraphicsTools/GPUUploadManagerPageTest.cpp‎

Lines changed: 53 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@ static const size_t kNumThreads = std::max(4u, std::thread::hardware_concurrency
4545
TEST(GPUUploadManagerPageTest, States)
4646
{
4747
{
48-
GPUUploadManagerImpl::Page Page{0};
49-
EXPECT_TRUE(Page.TryBeginWriting()) << "Should be able to begin writing to a new page";
50-
EXPECT_TRUE(Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed after the first writer finishes";
48+
GPUUploadManagerImpl::Page Page{0};
49+
GPUUploadManagerImpl::Page::Writer Writer = Page.TryBeginWriting();
50+
EXPECT_TRUE(Writer) << "Should be able to begin writing to a new page";
51+
EXPECT_TRUE(Writer.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed after the first writer finishes";
5152
}
5253

5354
{
@@ -57,44 +58,50 @@ TEST(GPUUploadManagerPageTest, States)
5758
EXPECT_FALSE(Page.TryBeginWriting()) << "Should not be able to begin writing to a sealed page";
5859

5960
Page.Reset(nullptr);
60-
EXPECT_TRUE(Page.TryBeginWriting()) << "Should be able to begin writing after resetting the page";
61-
EXPECT_EQ(Page.EndWriting(), GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed after the first writer finishes";
61+
GPUUploadManagerImpl::Page::Writer Writer = Page.TryBeginWriting();
62+
EXPECT_TRUE(Writer) << "Should be able to begin writing after resetting the page";
63+
EXPECT_EQ(Writer.EndWriting(), GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed after the first writer finishes";
6264
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be ready immediately";
6365
}
6466

6567
{
66-
GPUUploadManagerImpl::Page Page{0};
67-
EXPECT_TRUE(Page.TryBeginWriting()) << "Should be able to begin writing to a new page";
68-
EXPECT_TRUE(Page.TryBeginWriting());
68+
GPUUploadManagerImpl::Page Page{0};
69+
GPUUploadManagerImpl::Page::Writer Writer1 = Page.TryBeginWriting();
70+
GPUUploadManagerImpl::Page::Writer Writer2 = Page.TryBeginWriting();
71+
72+
EXPECT_TRUE(Writer1) << "Should be able to begin writing to a new page";
73+
EXPECT_TRUE(Writer2);
6974
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::NotReady) << "Page with active writers should not be ready";
70-
EXPECT_TRUE(Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotLastWriter) << "Writer should not be the last one to finish";
71-
EXPECT_TRUE(Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::LastWriterSealed) << "Writer should be the last one to finish";
75+
EXPECT_TRUE(Writer1.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotLastWriter) << "Writer should not be the last one to finish";
76+
EXPECT_TRUE(Writer2.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::LastWriterSealed) << "Writer should be the last one to finish";
7277
}
7378

7479
{
75-
GPUUploadManagerImpl::Page Page{1024};
76-
EXPECT_TRUE(Page.TryBeginWriting()) << "Should be able to begin writing to a new page";
77-
EXPECT_TRUE(Page.ScheduleBufferUpdate(nullptr, 0, 512, nullptr, nullptr, nullptr));
78-
EXPECT_TRUE(Page.ScheduleBufferUpdate(nullptr, 512, 512, nullptr, nullptr, nullptr));
79-
EXPECT_FALSE(Page.ScheduleBufferUpdate(nullptr, 1024, 512, nullptr, nullptr, nullptr)) << "Should not be able to schedule an update that exceeds the page size";
80+
GPUUploadManagerImpl::Page Page{1024};
81+
GPUUploadManagerImpl::Page::Writer Writer = Page.TryBeginWriting();
82+
EXPECT_TRUE(Writer) << "Should be able to begin writing to a new page";
83+
EXPECT_TRUE(Writer.ScheduleBufferUpdate(nullptr, 0, 512, nullptr, nullptr, nullptr));
84+
EXPECT_TRUE(Writer.ScheduleBufferUpdate(nullptr, 512, 512, nullptr, nullptr, nullptr));
85+
EXPECT_FALSE(Writer.ScheduleBufferUpdate(nullptr, 1024, 512, nullptr, nullptr, nullptr)) << "Should not be able to schedule an update that exceeds the page size";
8086
EXPECT_EQ(Page.GetNumPendingOps(), size_t{2});
81-
EXPECT_TRUE(Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed";
87+
EXPECT_TRUE(Writer.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed";
8288
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be ready immediately";
8389
EXPECT_EQ(Page.GetNumPendingOps(), size_t{2});
8490
Page.ExecutePendingOps(nullptr, 0);
8591
EXPECT_EQ(Page.GetNumPendingOps(), size_t{0});
8692
}
8793

8894
{
89-
GPUUploadManagerImpl::Page Page{1024};
90-
EXPECT_TRUE(Page.TryBeginWriting()) << "Should be able to begin writing to a new page";
91-
EXPECT_TRUE(Page.ScheduleBufferUpdate(nullptr, 0, 512, nullptr, nullptr, nullptr));
92-
EXPECT_FALSE(Page.ScheduleBufferUpdate(nullptr, 0, 1024, nullptr, nullptr, nullptr));
93-
EXPECT_TRUE(Page.ScheduleBufferUpdate(nullptr, 0, 256, nullptr, nullptr, nullptr));
94-
EXPECT_FALSE(Page.ScheduleBufferUpdate(nullptr, 0, 512, nullptr, nullptr, nullptr));
95-
EXPECT_TRUE(Page.ScheduleBufferUpdate(nullptr, 0, 256, nullptr, nullptr, nullptr));
96-
EXPECT_FALSE(Page.ScheduleBufferUpdate(nullptr, 0, 256, nullptr, nullptr, nullptr));
97-
EXPECT_EQ(Page.EndWriting(), GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed";
95+
GPUUploadManagerImpl::Page Page{1024};
96+
GPUUploadManagerImpl::Page::Writer Writer = Page.TryBeginWriting();
97+
EXPECT_TRUE(Writer) << "Should be able to begin writing to a new page";
98+
EXPECT_TRUE(Writer.ScheduleBufferUpdate(nullptr, 0, 512, nullptr, nullptr, nullptr));
99+
EXPECT_FALSE(Writer.ScheduleBufferUpdate(nullptr, 0, 1024, nullptr, nullptr, nullptr));
100+
EXPECT_TRUE(Writer.ScheduleBufferUpdate(nullptr, 0, 256, nullptr, nullptr, nullptr));
101+
EXPECT_FALSE(Writer.ScheduleBufferUpdate(nullptr, 0, 512, nullptr, nullptr, nullptr));
102+
EXPECT_TRUE(Writer.ScheduleBufferUpdate(nullptr, 0, 256, nullptr, nullptr, nullptr));
103+
EXPECT_FALSE(Writer.ScheduleBufferUpdate(nullptr, 0, 256, nullptr, nullptr, nullptr));
104+
EXPECT_EQ(Writer.EndWriting(), GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed";
98105
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be ready immediately";
99106
}
100107
}
@@ -117,19 +124,20 @@ TEST(GPUUploadManagerPageTest, ParallelTryBeginWriting)
117124
threads.emplace_back([&]() {
118125
StartSignal.Wait(true, static_cast<int>(kNumThreads));
119126

120-
Uint32 NumWrites = 0;
127+
std::vector<GPUUploadManagerImpl::Page::Writer> Writers;
128+
Writers.reserve(kNumIterations);
121129

122130
bool IsSealed = false;
123131
for (size_t i = 0; i < kNumIterations; ++i)
124132
{
125-
bool WriteStarted = Page.TryBeginWriting();
126-
if (WriteStarted)
127-
++NumWrites;
133+
GPUUploadManagerImpl::Page::Writer Writer = Page.TryBeginWriting();
134+
if (Writer)
135+
Writers.emplace_back(std::move(Writer));
128136
else
129137
IsSealed = true;
130138

131139
if (IsSealed)
132-
EXPECT_FALSE(WriteStarted) << "No writes can be started after the page is sealed";
140+
EXPECT_FALSE(Writer) << "No writes can be started after the page is sealed";
133141
}
134142

135143
if (Page.TrySeal() == GPUUploadManagerImpl::Page::SealStatus::AlreadySealed)
@@ -140,13 +148,13 @@ TEST(GPUUploadManagerPageTest, ParallelTryBeginWriting)
140148
EXPECT_FALSE(Page.TryBeginWriting()) << "No writes can be started after the page is sealed";
141149
}
142150

143-
for (size_t i = 0; i < NumWrites; ++i)
151+
for (GPUUploadManagerImpl::Page::Writer& Writer : Writers)
144152
{
145-
if (Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::LastWriterSealed)
153+
if (Writer.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::LastWriterSealed)
146154
NumLastWriters.fetch_add(1);
147155
}
148156

149-
TotalWrites.fetch_add(NumWrites);
157+
TotalWrites.fetch_add(static_cast<Uint32>(Writers.size()));
150158
});
151159
}
152160

@@ -190,12 +198,17 @@ TEST(GPUUploadManagerPageTest, NoWritesAfterSeal)
190198
{
191199
if (IsSealed.load())
192200
{
193-
EXPECT_FALSE(Page.TryBeginWriting()) << "No writes can be started after the page is sealed";
201+
GPUUploadManagerImpl::Page::Writer Writer = Page.TryBeginWriting();
202+
EXPECT_FALSE(Writer) << "No writes can be started after the page is sealed";
194203
}
195204
else
196205
{
197-
if (Page.TryBeginWriting())
206+
GPUUploadManagerImpl::Page::Writer Writer = Page.TryBeginWriting();
207+
if (Writer)
208+
{
198209
NumWritesStarted.fetch_add(1);
210+
Writer.EndWriting();
211+
}
199212
}
200213
}
201214
}
@@ -237,16 +250,18 @@ TEST(GPUUploadManagerPageTest, ScheduleBufferUpdateParallel)
237250
threads.emplace_back(
238251
[&](size_t ThreadId) {
239252
StartSignal.Wait(true, static_cast<int>(kNumThreads));
240-
if (Page.TryBeginWriting())
253+
254+
GPUUploadManagerImpl::Page::Writer Writer = Page.TryBeginWriting();
255+
if (Writer)
241256
{
242257
for (size_t i = 0; i < kNumIterations; ++i)
243258
{
244-
if (Page.ScheduleBufferUpdate(nullptr, 0, kUpdateSize, nullptr, Callback, Callback))
259+
if (Writer.ScheduleBufferUpdate(nullptr, 0, kUpdateSize, nullptr, Callback, Callback))
245260
{
246261
UpdatesScheduled.fetch_add(1);
247262
}
248263
}
249-
Page.EndWriting();
264+
Writer.EndWriting();
250265
}
251266
},
252267
t);

0 commit comments

Comments
 (0)