Skip to content

Commit e765434

Browse files
authored
Merge pull request o3de#17533 from aws-lumberyard-dev/AssetPrioritySortCrash
Asset Sorting Crash Fix
2 parents e87c2da + 4715d30 commit e765434

File tree

5 files changed

+84
-2
lines changed

5 files changed

+84
-2
lines changed

Code/Framework/AzCore/AzCore/IO/Streamer/FileRequest.h

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -270,8 +270,12 @@ namespace AZ::IO::Requests
270270

271271
namespace AZ::IO
272272
{
273+
class Streamer_SchedulerTest_RequestSorting_Test;
274+
273275
class FileRequest final
274276
{
277+
friend Streamer_SchedulerTest_RequestSorting_Test;
278+
275279
public:
276280
inline constexpr static AZStd::chrono::steady_clock::time_point s_noDeadlineTime = AZStd::chrono::steady_clock::time_point::max();
277281

@@ -407,6 +411,7 @@ namespace AZ::IO
407411
friend class StreamerContext;
408412
friend class Scheduler;
409413
friend class Device;
414+
friend class Streamer_SchedulerTest_RequestSorting_Test;
410415
friend bool operator==(const FileRequestHandle& lhs, const FileRequestPtr& rhs);
411416

412417
public:

Code/Framework/AzCore/AzCore/IO/Streamer/Scheduler.cpp

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -501,6 +501,11 @@ namespace AZ::IO
501501

502502
auto Scheduler::Thread_PrioritizeRequests(const FileRequest* first, const FileRequest* second) const -> Order
503503
{
504+
if (first == second)
505+
{
506+
return Order::Equal;
507+
}
508+
504509
// Sort by order priority of the command in the request. This allows to for instance have cancel request
505510
// always happen before any other requests.
506511
auto order = [](auto&& args)
@@ -544,7 +549,12 @@ namespace AZ::IO
544549
}
545550

546551
// If neither has started and have the same priority, prefer to start the closest deadline.
547-
return firstRead->m_deadline <= secondRead->m_deadline ? Order::FirstRequest : Order::SecondRequest;
552+
if (firstRead->m_deadline == secondRead->m_deadline)
553+
{
554+
return Order::Equal;
555+
}
556+
557+
return firstRead->m_deadline < secondRead->m_deadline ? Order::FirstRequest : Order::SecondRequest;
548558
}
549559

550560
// Check if one of the requests is in panic and prefer to prioritize that request
@@ -593,7 +603,13 @@ namespace AZ::IO
593603
s64 secondReadOffset = AZStd::visit(offset, second->GetCommand());
594604
s64 firstSeekDistance = abs(aznumeric_cast<s64>(m_threadData.m_lastFileOffset) - firstReadOffset);
595605
s64 secondSeekDistance = abs(aznumeric_cast<s64>(m_threadData.m_lastFileOffset) - secondReadOffset);
596-
return firstSeekDistance <= secondSeekDistance ? Order::FirstRequest : Order::SecondRequest;
606+
607+
if (firstSeekDistance == secondSeekDistance)
608+
{
609+
return Order::Equal;
610+
}
611+
612+
return firstSeekDistance < secondSeekDistance ? Order::FirstRequest : Order::SecondRequest;
597613
}
598614

599615
// Prefer to continue in the same file so prioritize the request that's in the same file
@@ -625,6 +641,13 @@ namespace AZ::IO
625641
"Scheduler::Thread_ScheduleRequests - Sorting %i requests", m_context.GetNumPreparedRequests());
626642
auto sorter = [this](const FileRequest* lhs, const FileRequest* rhs) -> bool
627643
{
644+
if (lhs == rhs)
645+
{
646+
// AZStd::sort may compare an element to itself;
647+
// it's required this condition remain consistent and return false.
648+
return false;
649+
}
650+
628651
Order order = Thread_PrioritizeRequests(lhs, rhs);
629652
switch (order)
630653
{

Code/Framework/AzCore/AzCore/IO/Streamer/Scheduler.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
namespace AZ::IO
2525
{
2626
class FileRequest;
27+
class Streamer_SchedulerTest_RequestSorting_Test;
2728

2829
namespace Requests
2930
{
@@ -65,6 +66,7 @@ namespace AZ::IO
6566
void GetRecommendations(IStreamerTypes::Recommendations& recommendations) const;
6667

6768
private:
69+
friend class Streamer_SchedulerTest_RequestSorting_Test;
6870
inline static constexpr u32 ProfilerColor = 0x0080ffff; //!< A lite shade of blue. (See https://www.color-hex.com/color/0080ff).
6971

7072
void Thread_MainLoop();

Code/Framework/AzCore/AzCore/IO/Streamer/Streamer.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,13 +28,15 @@ namespace AZ::IO::Requests
2828
namespace AZ::IO
2929
{
3030
class StreamStackEntry;
31+
class Streamer_SchedulerTest_RequestSorting_Test;
3132

3233
/**
3334
* Data streamer.
3435
*/
3536
class Streamer final
3637
: public AZ::IO::IStreamer
3738
{
39+
friend Streamer_SchedulerTest_RequestSorting_Test;
3840
public:
3941
AZ_RTTI(Streamer, "{3D880982-6E3F-4913-9947-55E01030D4AA}", IStreamer);
4042
AZ_CLASS_ALLOCATOR(Streamer, SystemAllocator);

Code/Framework/AzCore/Tests/Streamer/SchedulerTests.cpp

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -378,4 +378,54 @@ namespace AZ::IO
378378

379379
EXPECT_EQ(Iterations + 1, counter);
380380
}
381+
382+
TEST_F(Streamer_SchedulerTest, RequestSorting)
383+
{
384+
//////////////////////////////////////////////////////////////
385+
// Test equal priority requests that are past their deadlines (aka panic)
386+
//////////////////////////////////////////////////////////////
387+
IStreamerTypes::Deadline panicDeadline(IStreamerTypes::Deadline::min());
388+
auto estimatedCompleteTime = AZStd::chrono::steady_clock::now();
389+
char fakeBuffer[8];
390+
FileRequestPtr panicRequest = m_streamer->Read("PanicRequest", fakeBuffer, sizeof(fakeBuffer), 8, panicDeadline);
391+
panicRequest->m_request.SetEstimatedCompletion(estimatedCompleteTime);
392+
393+
// Passed deadline, same object (same pointer)
394+
EXPECT_EQ(
395+
m_streamer->m_streamStack->Thread_PrioritizeRequests(&panicRequest->m_request, &panicRequest->m_request),
396+
Scheduler::Order::Equal);
397+
398+
// Passed deadline, different object
399+
FileRequestPtr panicRequest2 = m_streamer->Read("PanicRequest2", fakeBuffer, sizeof(fakeBuffer), 8, panicDeadline);
400+
panicRequest2->m_request.SetEstimatedCompletion(estimatedCompleteTime);
401+
EXPECT_EQ(
402+
m_streamer->m_streamStack->Thread_PrioritizeRequests(&panicRequest->m_request, &panicRequest2->m_request),
403+
Scheduler::Order::Equal);
404+
405+
406+
//////////////////////////////////////////////////////////////
407+
// Test equal priority requests that are both reading the same file
408+
//////////////////////////////////////////////////////////////
409+
FileRequestPtr readRequest = m_streamer->Read("SameFile", fakeBuffer, sizeof(fakeBuffer), 8, panicDeadline);
410+
FileRequestPtr sameFileRequest = m_streamer->CreateRequest();
411+
sameFileRequest->m_request.CreateRead(&sameFileRequest->m_request, fakeBuffer, 8, RequestPath(), 0, 8);
412+
sameFileRequest->m_request.m_parent = &readRequest->m_request;
413+
sameFileRequest->m_request.m_dependencies = 0;
414+
415+
// Same file read, same object (same pointer)
416+
EXPECT_EQ(
417+
m_streamer->m_streamStack->Thread_PrioritizeRequests(&sameFileRequest->m_request, &sameFileRequest->m_request),
418+
Scheduler::Order::Equal);
419+
420+
FileRequestPtr readRequest2 = m_streamer->Read("SameFile2", fakeBuffer, sizeof(fakeBuffer), 8, panicDeadline);
421+
FileRequestPtr sameFileRequest2 = m_streamer->CreateRequest();
422+
sameFileRequest2->m_request.CreateRead(&sameFileRequest2->m_request, fakeBuffer, 8, RequestPath(), 0, 8);
423+
sameFileRequest2->m_request.m_parent = &readRequest2->m_request;
424+
sameFileRequest2->m_request.m_dependencies = 0;
425+
426+
// Same file read, different objects
427+
EXPECT_EQ(
428+
m_streamer->m_streamStack->Thread_PrioritizeRequests(&sameFileRequest->m_request, &sameFileRequest2->m_request),
429+
Scheduler::Order::Equal);
430+
}
381431
} // namespace AZ::IO

0 commit comments

Comments
 (0)