Skip to content

Commit

Permalink
LRUCache : Simplify handling of Handle::isWritable()
Browse files Browse the repository at this point in the history 
Now that WorkerRead recursion has been removed, writability is determined entirely by AquireMode, so we can replace conditionals with asserts.
  • Loading branch information
@johnhaddon
johnhaddon committed Oct 25, 2023
1 parent 34251d0 commit 06dd220
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 119 deletions.
38 changes: 14 additions & 24 deletions include/Gaffer/Private/IECorePreview/LRUCache.inl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,21 +167,12 @@ class Serial
return m_it->cacheEntry;
}

// Returns true if it is OK to call `writable()`.
// This is typically determined by the AcquireMode
// passed to `acquire()`, with special cases for
// recursion.
// Returns true if it is OK to call `writable()`. This is determined
// by the AcquireMode passed to `acquire()`.
bool isWritable() const
{
// Because this policy is serial, it would technically
// always be OK to write. But we return false for recursive
// calls to avoid unnecessary overhead updating the LRU list
// for inner calls.
/// \todo Is this distinction worth it, and do we really need
/// to support recursion on a single item in the Serial policy?
/// This is a remnant of a more complex system that allowed recursion
/// in the TaskParallel policy, but that has since been removed.
return m_it->handleCount == 1;
// Because this policy is serial, it is always OK to write
return true;
}

// Executes the functor F. This is used to
Expand Down Expand Up @@ -1093,6 +1084,7 @@ Value LRUCache<Key, Value, Policy, GetterKey>::get( const GetterKey &key, const

if( status==Uncached )
{
assert( handle.isWritable() );
Value value = Value();
Cost cost = 0;
try
Expand All @@ -1105,24 +1097,21 @@ Value LRUCache<Key, Value, Policy, GetterKey>::get( const GetterKey &key, const
}
catch( ... )
{
if( handle.isWritable() && m_cacheErrors )
if( m_cacheErrors )
{
handle.writable().state = std::current_exception();
}
throw;
}

if( handle.isWritable() )
{
assert( cacheEntry.status() != Cached ); // this would indicate that another thread somehow
assert( cacheEntry.status() != Failed ); // loaded the same thing as us, which is not the intention.
assert( cacheEntry.status() != Cached ); // this would indicate that another thread somehow
assert( cacheEntry.status() != Failed ); // loaded the same thing as us, which is not the intention.

setInternal( key, handle.writable(), value, cost );
m_policy.push( handle );
setInternal( key, handle.writable(), value, cost );
m_policy.push( handle );

handle.release();
limitCost( m_maxCost );
}
handle.release();
limitCost( m_maxCost );

return value;
}
Expand Down Expand Up @@ -1187,8 +1176,9 @@ bool LRUCache<Key, Value, Policy, GetterKey>::setIfUncached( const Key &key, con
const Status status = cacheEntry.status();

bool result = false;
if( status == Uncached && handle.isWritable() )
if( status == Uncached )
{
assert( handle.isWritable() );
result = setInternal( key, handle.writable(), value, costFunction( value ) );
m_policy.push( handle );

Expand Down
8 changes: 0 additions & 8 deletions python/GafferTest/IECorePreviewTest/LRUCacheTest.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,6 @@ def testRecursionWithEvictionsTaskParallel( self ) :

GafferTest.testLRUCacheRecursion( "taskParallel", numIterations = 100000, numValues = 1000, maxCost = 100 )

def testRecursionOnOneItemSerial( self ) :

GafferTest.testLRUCacheRecursionOnOneItem( "serial" )

def testClearFromGetSerial( self ) :

GafferTest.testLRUCacheClearFromGet( "serial" )
Expand Down Expand Up @@ -232,9 +228,5 @@ def testSetIfUncached( self ) :
with self.subTest( policy = policy ) :
GafferTest.testLRUCacheSetIfUncached( policy )

def testSetIfUncachedRecursion( self ) :

GafferTest.testLRUCacheSetIfUncachedRecursion( "serial" )

if __name__ == "__main__":
unittest.main()
87 changes: 0 additions & 87 deletions src/GafferTestModule/LRUCacheTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -308,53 +308,6 @@ void testLRUCacheRecursion( const std::string &policy, int numIterations, size_t
DispatchTest<TestLRUCacheRecursion>()( policy, numIterations, numValues, maxCost );
}

template<template<typename> class Policy>
struct TestLRUCacheRecursionOnOneItem
{

void operator()()
{
using Cache = LRUCache<int, int, Policy>;
using CachePtr = std::unique_ptr<Cache>;
int recursionDepth = 0;

CachePtr cache;
cache.reset(
new Cache(
// Getter that calls back into the cache with the _same_
// key, up to a certain limit, and then actually returns
// a value. This is basically insane, but it models
// situations that can occur in Gaffer.
[&cache, &recursionDepth]( int key, size_t &cost, const IECore::Canceller *canceller ) {
cost = 1;
if( ++recursionDepth == 100 )
{
return key;
}
else
{
return cache->get( key );
}
},
// Max cost is small enough that we'll be trying to evict
// keys while unwinding the recursion.
20
)
);

GAFFERTEST_ASSERTEQUAL( cache->currentCost(), 0 );
GAFFERTEST_ASSERTEQUAL( cache->get( 1 ), 1 );
GAFFERTEST_ASSERTEQUAL( recursionDepth, 100 );
GAFFERTEST_ASSERTEQUAL( cache->currentCost(), 1 );
}

};

void testLRUCacheRecursionOnOneItem( const std::string &policy )
{
DispatchTest<TestLRUCacheRecursionOnOneItem>()( policy );
}

template<template<typename> class Policy>
struct TestLRUCacheClearFromGet
{
Expand Down Expand Up @@ -847,44 +800,6 @@ void testLRUCacheSetIfUncached( const std::string &policy )
DispatchTest<TestLRUCacheSetIfUncached>()( policy );
}

template<template<typename> class Policy>
struct TestLRUCacheSetIfUncachedRecursion
{

void operator()()
{
using Cache = LRUCache<int, int, Policy>;
using CachePtr = std::unique_ptr<Cache>;

CachePtr cache;
cache.reset(
new Cache(
// Getter that calls `setIfUncached()` with the _same_ key. This
// is basically insane, but it models situations that can occur
// in Gaffer.
[&cache]( int key, size_t &cost, const IECore::Canceller *canceller ) {
cost = 1;
// We expect the call to fail, because the lock is held by the
// outer call to `get()`.
GAFFERTEST_ASSERT( !cache->setIfUncached( key, key, []( int ) { return 1; } ) );
return key;
},
1000
)
);

GAFFERTEST_ASSERTEQUAL( cache->currentCost(), 0 );
GAFFERTEST_ASSERTEQUAL( cache->get( 1 ), 1 );
GAFFERTEST_ASSERTEQUAL( cache->currentCost(), 1 );
}

};

void testLRUCacheSetIfUncachedRecursion( const std::string &policy )
{
DispatchTest<TestLRUCacheSetIfUncachedRecursion>()( policy );
}

} // namespace

void GafferTestModule::bindLRUCacheTest()
Expand All @@ -893,13 +808,11 @@ void GafferTestModule::bindLRUCacheTest()
def( "testLRUCacheRemovalCallback", &testLRUCacheRemovalCallback );
def( "testLRUCacheContentionForOneItem", &testLRUCacheContentionForOneItem, arg( "withCanceller" ) = false );
def( "testLRUCacheRecursion", &testLRUCacheRecursion, ( arg( "numIterations" ), arg( "numValues" ), arg( "maxCost" ) ) );
def( "testLRUCacheRecursionOnOneItem", &testLRUCacheRecursionOnOneItem );
def( "testLRUCacheClearFromGet", &testLRUCacheClearFromGet );
def( "testLRUCacheExceptions", &testLRUCacheExceptions );
def( "testLRUCacheCancellation", &testLRUCacheCancellation );
def( "testLRUCacheCancellationOfSecondGet", &testLRUCacheCancellationOfSecondGet );
def( "testLRUCacheUncacheableItem", &testLRUCacheUncacheableItem );
def( "testLRUCacheGetIfCached", &testLRUCacheGetIfCached );
def( "testLRUCacheSetIfUncached", &testLRUCacheSetIfUncached );
def( "testLRUCacheSetIfUncachedRecursion", &testLRUCacheSetIfUncachedRecursion );
}

0 comments on commit 06dd220

Please sign in to comment.