Skip to content

Commit 8311a9b

Browse files
TheMostDiligentTheMostDiligent
committed
GPUUploadManagerImpl::Page: improve offset calculation + cleanup
1 parent 3338993 commit 8311a9b

3 files changed

Lines changed: 40 additions & 24 deletions

File tree

‎Graphics/GraphicsTools/include/GPUUploadManagerImpl.hpp‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ class GPUUploadManagerImpl final : public ObjectBase<IGPUUploadManager>
6363
class Page
6464
{
6565
public:
66-
explicit Page(Uint32 Size);
66+
explicit Page(Uint32 Size) noexcept;
6767

6868
Page(IRenderDevice* pDevice,
6969
IDeviceContext* pContext,
@@ -110,7 +110,8 @@ class GPUUploadManagerImpl final : public ObjectBase<IGPUUploadManager>
110110
void ExecutePendingOps(IDeviceContext* pContext, Uint64 FenceValue);
111111
void Reset(IDeviceContext* pContext);
112112

113-
/// Tries to set the page as enqueued for execution. Returns true if the page was not previously enqueued, false otherwise.
113+
// Tries to set the page as enqueued for execution.
114+
// Returns true if the page was not previously enqueued, false otherwise.
114115
bool TryEnqueue();
115116

116117
Uint64 GetFenceValue() const { return m_FenceValue; }

‎Graphics/GraphicsTools/src/GPUUploadManagerImpl.cpp‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636
namespace Diligent
3737
{
3838

39-
GPUUploadManagerImpl::Page::Page(Uint32 Size) :
39+
GPUUploadManagerImpl::Page::Page(Uint32 Size) noexcept :
4040
m_Size{Size}
4141
{}
4242

@@ -113,11 +113,12 @@ GPUUploadManagerImpl::Page::SealStatus GPUUploadManagerImpl::Page::TrySeal()
113113
}
114114

115115
if (m_State.compare_exchange_weak(
116-
State, State | SEALED_BIT,
116+
State, // On failure, updated state is written back to State variable.
117+
State | SEALED_BIT,
117118
std::memory_order_acq_rel))
118119
{
119-
// If there are now writers at the instant we sealed the page, it's now stable:
120-
// no new writers can start.
120+
// If there were no writers at the instant we sealed the page,
121+
// it's now ready for execution because no new writers can start
121122
return (State & WRITER_MASK) == 0 ?
122123
SealStatus::Ready :
123124
SealStatus::NotReady;
@@ -135,15 +136,20 @@ bool GPUUploadManagerImpl::Page::ScheduleBufferUpdate(IBuffer*
135136
VERIFY_EXPR(GetWriterCount() > 0);
136137

137138
// Note that the page may be sealed for new writes at this point,
138-
// but we can still schedule the update since we have an active writer.
139+
// but we can still schedule the update since we have an active writer
140+
// that prevents the page from being submitted for execution.
139141

140-
constexpr Uint32 Alignment = 16;
141-
const Uint32 Offset = m_Offset.fetch_add(AlignUp(NumBytes, Alignment));
142-
VERIFY_EXPR(Offset % Alignment == 0);
142+
constexpr Uint32 Alignment = 16;
143+
const Uint32 AlignedSize = AlignUp(NumBytes, Alignment);
143144

144-
if (Offset + NumBytes > m_Size)
145+
Uint32 Offset = m_Offset.load(std::memory_order_relaxed);
146+
for (;;)
145147
{
146-
return false;
148+
if (Offset + AlignedSize > m_Size)
149+
return false; // Fail without incrementing offset
150+
151+
if (m_Offset.compare_exchange_weak(Offset, Offset + AlignedSize, std::memory_order_relaxed))
152+
break; // Success
147153
}
148154

149155
m_NumPendingOps.fetch_add(1, std::memory_order_relaxed);

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

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -52,23 +52,23 @@ TEST(GPUUploadManagerPageTest, States)
5252

5353
{
5454
GPUUploadManagerImpl::Page Page{0};
55-
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be sealed immediately";
55+
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be ready immediately";
5656
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::AlreadySealed) << "Sealing an already sealed page should return AlreadySealed";
5757
EXPECT_FALSE(Page.TryBeginWriting()) << "Should not be able to begin writing to a sealed page";
5858

5959
Page.Reset(nullptr);
6060
EXPECT_TRUE(Page.TryBeginWriting()) << "Should be able to begin writing after resetting the page";
6161
EXPECT_EQ(Page.EndWriting(), GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed after the first writer finishes";
62-
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be sealed immediately";
62+
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be ready immediately";
6363
}
6464

6565
{
6666
GPUUploadManagerImpl::Page Page{0};
6767
EXPECT_TRUE(Page.TryBeginWriting()) << "Should be able to begin writing to a new page";
6868
EXPECT_TRUE(Page.TryBeginWriting());
69-
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::NotReady) << "Page with active writers should not be ready immediately after sealing";
70-
EXPECT_TRUE(Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotLastWriter) << "Page should not be sealed after the first writer finishes";
71-
EXPECT_TRUE(Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::LastWriterSealed) << "Page should be sealed after the last writer finishes";
69+
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";
7272
}
7373

7474
{
@@ -78,8 +78,8 @@ TEST(GPUUploadManagerPageTest, States)
7878
EXPECT_TRUE(Page.ScheduleBufferUpdate(nullptr, 512, 512, nullptr, nullptr, nullptr));
7979
EXPECT_FALSE(Page.ScheduleBufferUpdate(nullptr, 1024, 512, nullptr, nullptr, nullptr)) << "Should not be able to schedule an update that exceeds the page size";
8080
EXPECT_EQ(Page.GetNumPendingOps(), size_t{2});
81-
EXPECT_TRUE(Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed after the first writer finishes";
82-
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be sealed immediately";
81+
EXPECT_TRUE(Page.EndWriting() == GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed";
82+
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be ready immediately";
8383
EXPECT_EQ(Page.GetNumPendingOps(), size_t{2});
8484
Page.ExecutePendingOps(nullptr, 0);
8585
EXPECT_EQ(Page.GetNumPendingOps(), size_t{0});
@@ -88,9 +88,14 @@ TEST(GPUUploadManagerPageTest, States)
8888
{
8989
GPUUploadManagerImpl::Page Page{1024};
9090
EXPECT_TRUE(Page.TryBeginWriting()) << "Should be able to begin writing to a new page";
91-
EXPECT_FALSE(Page.ScheduleBufferUpdate(nullptr, 0, 4096, nullptr, nullptr, nullptr)) << "Should not be able to schedule an update that exceeds the page size";
92-
EXPECT_FALSE(Page.ScheduleBufferUpdate(nullptr, 0, 128, nullptr, nullptr, nullptr)) << "Should not be able to schedule an update since the offset should be past the page size";
93-
EXPECT_EQ(Page.EndWriting(), GPUUploadManagerImpl::Page::WritingStatus::NotSealed) << "Page should not be sealed after the first writer finishes";
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";
98+
EXPECT_EQ(Page.TrySeal(), GPUUploadManagerImpl::Page::SealStatus::Ready) << "Page with no active writers should be ready immediately";
9499
}
95100
}
96101

@@ -167,7 +172,8 @@ TEST(GPUUploadManagerPageTest, NoWritesAfterSeal)
167172

168173
static constexpr size_t kNumIterations = 1000;
169174

170-
std::atomic<bool> IsSealed{false};
175+
std::atomic<bool> IsSealed{false};
176+
std::atomic<size_t> NumWritesStarted{0};
171177
for (size_t t = 0; t < kNumThreads; ++t)
172178
{
173179
threads.emplace_back(
@@ -188,7 +194,8 @@ TEST(GPUUploadManagerPageTest, NoWritesAfterSeal)
188194
}
189195
else
190196
{
191-
Page.TryBeginWriting();
197+
if (Page.TryBeginWriting())
198+
NumWritesStarted.fetch_add(1);
192199
}
193200
}
194201
}
@@ -200,6 +207,8 @@ TEST(GPUUploadManagerPageTest, NoWritesAfterSeal)
200207

201208
for (auto& thread : threads)
202209
thread.join();
210+
211+
LOG_INFO_MESSAGE("Number of writes started before the page was sealed: ", NumWritesStarted.load());
203212
}
204213

205214

0 commit comments

Comments
 (0)