From e9a9b87381b8c9e25975f454a74701a2333b2fcb Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Fri, 12 Apr 2024 15:49:07 +0200 Subject: [PATCH 1/5] [macOS] FlutterSurfaceManager should not return surfaces that are in use --- .../macos/framework/Source/FlutterSurface.h | 8 ++ .../macos/framework/Source/FlutterSurface.mm | 23 +++++ .../framework/Source/FlutterSurfaceManager.h | 2 +- .../framework/Source/FlutterSurfaceManager.mm | 46 +++++++-- .../Source/FlutterSurfaceManagerTest.mm | 99 +++++++++++++++++++ 5 files changed, 167 insertions(+), 11 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurface.h b/shell/platform/darwin/macos/framework/Source/FlutterSurface.h index b566e68092dd3..9fd3557661e37 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurface.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurface.h @@ -32,7 +32,15 @@ @property(readonly, nonatomic, nonnull) IOSurfaceRef ioSurface; @property(readonly, nonatomic) CGSize size; @property(readonly, nonatomic) int64_t textureId; +// Age of the surface in frames. 0 means just returned from the compositor. +@property(readwrite, nonatomic) int age; +// Whether the surface is currently in use by the compositor. +@property(readonly, nonatomic) BOOL isInUse; @end +@interface FlutterSurface (Testing) +@property(readwrite, nonatomic) BOOL isInUseOverride; +@end + #endif // FLUTTER_SHELL_PLATFORM_DARWIN_MACOS_FRAMEWORK_SOURCE_FLUTTERSURFACE_H_ diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm index 4b65f2553c315..b9574aef57de3 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm @@ -10,11 +10,22 @@ @interface FlutterSurface () { CGSize _size; IOSurfaceRef _ioSurface; id _texture; + int _age; + // Used for testing. + BOOL _isInUseOverride; } @end @implementation FlutterSurface +- (void)setAge:(int)age { + self->_age = age; +} + +- (int)age { + return _age; +} + - (IOSurfaceRef)ioSurface { return _ioSurface; } @@ -27,6 +38,18 @@ - (int64_t)textureId { return reinterpret_cast(_texture); } +- (BOOL)isInUse { + return _isInUseOverride || IOSurfaceIsInUse(_ioSurface); +} + +- (BOOL)isInUseOverride { + return _isInUseOverride; +} + +- (void)setIsInUseOverride:(BOOL)isInUseOverride { + _isInUseOverride = isInUseOverride; +} + - (instancetype)initWithSize:(CGSize)size device:(id)device { if (self = [super init]) { self->_size = size; diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h index 279d580e2f4cd..d515741538bc0 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.h @@ -88,7 +88,7 @@ /** * Removes all cached surfaces replacing them with new ones. */ -- (void)replaceSurfaces:(nonnull NSArray*)surfaces; +- (void)returnSurfaces:(nonnull NSArray*)surfaces; /** * Returns number of surfaces currently in cache. Used for tests. diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm index bed88bf581082..25755887a5e07 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm @@ -153,7 +153,7 @@ - (void)commit:(NSArray*)surfaces { FML_DCHECK([NSThread isMainThread]); // Release all unused back buffer surfaces and replace them with front surfaces. - [_backBufferCache replaceSurfaces:_frontSurfaces]; + [_backBufferCache returnSurfaces:_frontSurfaces]; // Front surfaces will be replaced by currently presented surfaces. [_frontSurfaces removeAllObjects]; @@ -284,22 +284,48 @@ - (instancetype)init { - (nullable FlutterSurface*)removeSurfaceForSize:(CGSize)size { @synchronized(self) { + // Purge all cached surfaces if the size has changed. + if (_surfaces.firstObject != nil && !CGSizeEqualToSize(_surfaces.firstObject.size, size)) { + [_surfaces removeAllObjects]; + } + + FlutterSurface* res; + + // Returns youngest surface that is not in use. Inside [returnSurfaces:] + // surfaces over certain age are purged, returning youngest surface ensures + // that the cache doesn't keep more surfaces than it needs to. for (FlutterSurface* surface in _surfaces) { - if (CGSizeEqualToSize(surface.size, size)) { - // By default ARC doesn't retain enumeration iteration variables. - FlutterSurface* res = surface; - [_surfaces removeObject:surface]; - return res; + if (!surface.isInUse && (res == nil || res.age > surface.age)) { + res = surface; } } - return nil; + if (res != nil) { + [_surfaces removeObject:res]; + } + return res; } } -- (void)replaceSurfaces:(nonnull NSArray*)surfaces { +- (void)returnSurfaces:(nonnull NSArray*)returnedSurfaces { @synchronized(self) { - [_surfaces removeAllObjects]; - [_surfaces addObjectsFromArray:surfaces]; + for (FlutterSurface* surface in returnedSurfaces) { + surface.age = 0; + } + for (FlutterSurface* surface in _surfaces) { + ++surface.age; + } + + [_surfaces addObjectsFromArray:returnedSurfaces]; + + // Purge all surface with age > 4. Reaching this age can mean two things: + // - Surface is still in use and we can't return it. This can happen in some edge + // cases where the compositor holds on to the surface for much longer than expected. + // - Surface is not in use but it hasn't been requested from the cache for a while. + // This means there are too many surfaces in the cache. + [_surfaces filterUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(FlutterSurface* surface, + NSDictionary* bindings) { + return surface.age <= 4; + }]]; } // performSelector:withObject:afterDelay needs to be performed on RunLoop thread diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm index 04471eb914863..73faf6a03f5f0 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm @@ -117,6 +117,21 @@ - (void)onPresent:(CGSize)frameSize withBlock:(nonnull dispatch_block_t)block { auto surfaceFromCache = [surfaceManager surfaceForSize:CGSizeMake(110, 110)]; EXPECT_EQ(surfaceFromCache, surface2); + // Submit empty surfaces until the one in cache gets to age > 4, in which case + // it should be removed. + + [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); + + [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); + + [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); + + [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); + [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); @@ -163,6 +178,90 @@ - (void)onPresent:(CGSize)frameSize withBlock:(nonnull dispatch_block_t)block { EXPECT_EQ(surface3, surface1); } +TEST(FlutterSurfaceManager, BackingStoreCacheSurfaceStuckInUse) { + TestView* testView = [[TestView alloc] init]; + FlutterSurfaceManager* surfaceManager = CreateSurfaceManager(testView); + + auto surface1 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + + [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface1) ] atTime:0 notify:nil]; + // Pretend that compositor is holding on to the surface. The surface will be kept + // in cache until the age of 5 is reached, and then evicted. + surface1.isInUseOverride = YES; + + auto surface2 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface2) ] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); + + auto surface3 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface3) ] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); + + auto surface4 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface4) ] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); + + auto surface5 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface5) ] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); + + auto surface6 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface6) ] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); + + auto surface7 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface7) ] atTime:0 notify:nil]; + // Surface in use should bet old enough at this point to be evicted. + EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); +} + +TEST(FlutterSurfaceManager, BackingStoreClampsNumberOfBuffers) { + TestView* testView = [[TestView alloc] init]; + FlutterSurfaceManager* surfaceManager = CreateSurfaceManager(testView); + + auto surface1 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + auto surface2 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + auto surface3 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + + [surfaceManager presentSurfaces:@[ + CreatePresentInfo(surface1), + CreatePresentInfo(surface2), + CreatePresentInfo(surface3), + ] + atTime:0 + notify:nil]; + + auto surface4 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + auto surface5 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + auto surface6 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + + [surfaceManager presentSurfaces:@[ + CreatePresentInfo(surface4), + CreatePresentInfo(surface5), + CreatePresentInfo(surface6), + ] + atTime:0 + notify:nil]; + + EXPECT_EQ(surfaceManager.backBufferCache.count, 3ul); + EXPECT_EQ(surfaceManager.frontSurfaces.count, 3ul); + + auto surface7 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + auto surface8 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + + [surfaceManager presentSurfaces:@[ + CreatePresentInfo(surface7), + CreatePresentInfo(surface8), + ] + atTime:0 + notify:nil]; + + // Number of back buffers gets trimmed to number of front surfaces. + // 2 buffers for age == 0 and 2 buffers for age == 1. + EXPECT_EQ(surfaceManager.backBufferCache.count, 4ul); + EXPECT_EQ(surfaceManager.frontSurfaces.count, 2ul); +} + inline bool operator==(const CGRect& lhs, const CGRect& rhs) { return CGRectEqualToRect(lhs, rhs); } From dc324f41d2857dd68d125a1413aae42ad218a2b5 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Fri, 12 Apr 2024 20:20:55 +0200 Subject: [PATCH 2/5] Remove obsolete test --- .../Source/FlutterSurfaceManagerTest.mm | 47 ------------------- 1 file changed, 47 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm index 73faf6a03f5f0..7f463e0337214 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm @@ -215,53 +215,6 @@ - (void)onPresent:(CGSize)frameSize withBlock:(nonnull dispatch_block_t)block { EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); } -TEST(FlutterSurfaceManager, BackingStoreClampsNumberOfBuffers) { - TestView* testView = [[TestView alloc] init]; - FlutterSurfaceManager* surfaceManager = CreateSurfaceManager(testView); - - auto surface1 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - auto surface2 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - auto surface3 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - - [surfaceManager presentSurfaces:@[ - CreatePresentInfo(surface1), - CreatePresentInfo(surface2), - CreatePresentInfo(surface3), - ] - atTime:0 - notify:nil]; - - auto surface4 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - auto surface5 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - auto surface6 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - - [surfaceManager presentSurfaces:@[ - CreatePresentInfo(surface4), - CreatePresentInfo(surface5), - CreatePresentInfo(surface6), - ] - atTime:0 - notify:nil]; - - EXPECT_EQ(surfaceManager.backBufferCache.count, 3ul); - EXPECT_EQ(surfaceManager.frontSurfaces.count, 3ul); - - auto surface7 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - auto surface8 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - - [surfaceManager presentSurfaces:@[ - CreatePresentInfo(surface7), - CreatePresentInfo(surface8), - ] - atTime:0 - notify:nil]; - - // Number of back buffers gets trimmed to number of front surfaces. - // 2 buffers for age == 0 and 2 buffers for age == 1. - EXPECT_EQ(surfaceManager.backBufferCache.count, 4ul); - EXPECT_EQ(surfaceManager.frontSurfaces.count, 2ul); -} - inline bool operator==(const CGRect& lhs, const CGRect& rhs) { return CGRectEqualToRect(lhs, rhs); } From 8d818072c0f336b80e25b6de542487a35910ca4b Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Fri, 12 Apr 2024 20:46:30 +0200 Subject: [PATCH 3/5] reword comment --- .../darwin/macos/framework/Source/FlutterSurfaceManager.mm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm index 25755887a5e07..192d47b1294ce 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm @@ -291,9 +291,9 @@ - (nullable FlutterSurface*)removeSurfaceForSize:(CGSize)size { FlutterSurface* res; - // Returns youngest surface that is not in use. Inside [returnSurfaces:] - // surfaces over certain age are purged, returning youngest surface ensures - // that the cache doesn't keep more surfaces than it needs to. + // Returns youngest surface that is not in use. Returning youngest surface ensures + // that the cache doesn't keep more surfaces than it needs to, as the unused surfaces + // kept in cache will have their age kept increasing until purged (inside [returnSurfaces:]). for (FlutterSurface* surface in _surfaces) { if (!surface.isInUse && (res == nil || res.age > surface.age)) { res = surface; From 21284e894edb567a376b043bd0261fa9bdb90d1d Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Wed, 17 Apr 2024 13:44:39 +0200 Subject: [PATCH 4/5] Move surface age to cache and reduce allocations. Increase max age to 30 to reduce potential surface allocations and deallocations. age of 30 is still only 500ms at 60 FPS, and there is idle timeout at 1 second which deallocates all surfaces. --- .../macos/framework/Source/FlutterSurface.h | 2 - .../macos/framework/Source/FlutterSurface.mm | 9 ---- .../framework/Source/FlutterSurfaceManager.mm | 24 ++++++++--- .../Source/FlutterSurfaceManagerTest.mm | 42 +++++-------------- 4 files changed, 30 insertions(+), 47 deletions(-) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurface.h b/shell/platform/darwin/macos/framework/Source/FlutterSurface.h index 9fd3557661e37..50f6a2ab32446 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurface.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurface.h @@ -32,8 +32,6 @@ @property(readonly, nonatomic, nonnull) IOSurfaceRef ioSurface; @property(readonly, nonatomic) CGSize size; @property(readonly, nonatomic) int64_t textureId; -// Age of the surface in frames. 0 means just returned from the compositor. -@property(readwrite, nonatomic) int age; // Whether the surface is currently in use by the compositor. @property(readonly, nonatomic) BOOL isInUse; diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm index b9574aef57de3..f61f81c5bf0b1 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm @@ -10,7 +10,6 @@ @interface FlutterSurface () { CGSize _size; IOSurfaceRef _ioSurface; id _texture; - int _age; // Used for testing. BOOL _isInUseOverride; } @@ -18,14 +17,6 @@ @interface FlutterSurface () { @implementation FlutterSurface -- (void)setAge:(int)age { - self->_age = age; -} - -- (int)age { - return _age; -} - - (IOSurfaceRef)ioSurface { return _ioSurface; } diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm index 192d47b1294ce..c26c5cc7e3f40 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm @@ -266,9 +266,12 @@ - (void)presentSurfaces:(NSArray*)surfaces // Cached back buffers will be released after kIdleDelay if there is no activity. static const double kIdleDelay = 1.0; +// Once surfaces reach kEvictionAge, they will be evicted from the cache. +static const int kSurfaceEvictionAge = 30; @interface FlutterBackBufferCache () { NSMutableArray* _surfaces; + NSMapTable* _surfaceAge; } @end @@ -278,10 +281,20 @@ @implementation FlutterBackBufferCache - (instancetype)init { if (self = [super init]) { self->_surfaces = [[NSMutableArray alloc] init]; + self->_surfaceAge = [NSMapTable weakToStrongObjectsMapTable]; } return self; } +- (int)ageForSurface:(FlutterSurface*)surface { + NSNumber* age = [_surfaceAge objectForKey:surface]; + return age != nil ? age.intValue : 0; +} + +- (void)setAge:(int)age forSurface:(FlutterSurface*)surface { + [_surfaceAge setObject:@(age) forKey:surface]; +} + - (nullable FlutterSurface*)removeSurfaceForSize:(CGSize)size { @synchronized(self) { // Purge all cached surfaces if the size has changed. @@ -295,7 +308,8 @@ - (nullable FlutterSurface*)removeSurfaceForSize:(CGSize)size { // that the cache doesn't keep more surfaces than it needs to, as the unused surfaces // kept in cache will have their age kept increasing until purged (inside [returnSurfaces:]). for (FlutterSurface* surface in _surfaces) { - if (!surface.isInUse && (res == nil || res.age > surface.age)) { + if (!surface.isInUse && + (res == nil || [self ageForSurface:res] > [self ageForSurface:surface])) { res = surface; } } @@ -309,22 +323,22 @@ - (nullable FlutterSurface*)removeSurfaceForSize:(CGSize)size { - (void)returnSurfaces:(nonnull NSArray*)returnedSurfaces { @synchronized(self) { for (FlutterSurface* surface in returnedSurfaces) { - surface.age = 0; + [self setAge:0 forSurface:surface]; } for (FlutterSurface* surface in _surfaces) { - ++surface.age; + [self setAge:[self ageForSurface:surface] + 1 forSurface:surface]; } [_surfaces addObjectsFromArray:returnedSurfaces]; - // Purge all surface with age > 4. Reaching this age can mean two things: + // Purge all surface with age = kSurfaceEvictionAge. Reaching this age can mean two things: // - Surface is still in use and we can't return it. This can happen in some edge // cases where the compositor holds on to the surface for much longer than expected. // - Surface is not in use but it hasn't been requested from the cache for a while. // This means there are too many surfaces in the cache. [_surfaces filterUsingPredicate:[NSPredicate predicateWithBlock:^BOOL(FlutterSurface* surface, NSDictionary* bindings) { - return surface.age <= 4; + return [self ageForSurface:surface] < kSurfaceEvictionAge; }]]; } diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm index 7f463e0337214..8d6996280e691 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm @@ -117,23 +117,13 @@ - (void)onPresent:(CGSize)frameSize withBlock:(nonnull dispatch_block_t)block { auto surfaceFromCache = [surfaceManager surfaceForSize:CGSizeMake(110, 110)]; EXPECT_EQ(surfaceFromCache, surface2); - // Submit empty surfaces until the one in cache gets to age > 4, in which case + // Submit empty surfaces until the one in cache gets to age >= kSurfaceEvictionAge, in which case // it should be removed. - [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); - - [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); - - [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); - - [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); - - [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); + for (int i = 0; i < 30 /* kSurfaceEvictionAge */; ++i) { + [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); + } [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; EXPECT_EQ(surfaceManager.backBufferCache.count, 0ul); @@ -186,31 +176,21 @@ - (void)onPresent:(CGSize)frameSize withBlock:(nonnull dispatch_block_t)block { [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface1) ] atTime:0 notify:nil]; // Pretend that compositor is holding on to the surface. The surface will be kept - // in cache until the age of 5 is reached, and then evicted. + // in cache until the age of kSurfaceEvictionAge is reached, and then evicted. surface1.isInUseOverride = YES; auto surface2 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface2) ] atTime:0 notify:nil]; EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); - auto surface3 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface3) ] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); + for (int i = 0; i < 30 /* kSurfaceEvictionAge */ - 1; ++i) { + auto surface3 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; + [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface3) ] atTime:0 notify:nil]; + EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); + } auto surface4 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface4) ] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); - - auto surface5 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface5) ] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); - - auto surface6 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface6) ] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 2ul); - - auto surface7 = [surfaceManager surfaceForSize:CGSizeMake(100, 100)]; - [surfaceManager presentSurfaces:@[ CreatePresentInfo(surface7) ] atTime:0 notify:nil]; // Surface in use should bet old enough at this point to be evicted. EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); } From d11e4da0ff54cf30d47e1018c2a2f6a947c51549 Mon Sep 17 00:00:00 2001 From: Matej Knopp Date: Wed, 17 Apr 2024 14:45:33 +0200 Subject: [PATCH 5/5] add comment --- .../darwin/macos/framework/Source/FlutterSurfaceManager.mm | 3 +++ 1 file changed, 3 insertions(+) diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm index c26c5cc7e3f40..d87aed4c9e4ce 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManager.mm @@ -267,6 +267,9 @@ - (void)presentSurfaces:(NSArray*)surfaces // Cached back buffers will be released after kIdleDelay if there is no activity. static const double kIdleDelay = 1.0; // Once surfaces reach kEvictionAge, they will be evicted from the cache. +// The age of 30 has been chosen to reduce potential surface allocation churn. +// For unused surface 30 frames means only half a second at 60fps, and there is +// idle timeout of 1 second where all surfaces are evicted. static const int kSurfaceEvictionAge = 30; @interface FlutterBackBufferCache () {