diff --git a/shell/platform/darwin/macos/framework/Source/FlutterSurface.h b/shell/platform/darwin/macos/framework/Source/FlutterSurface.h index b566e68092dd3..50f6a2ab32446 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurface.h +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurface.h @@ -32,7 +32,13 @@ @property(readonly, nonatomic, nonnull) IOSurfaceRef ioSurface; @property(readonly, nonatomic) CGSize size; @property(readonly, nonatomic) int64_t textureId; +// 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..f61f81c5bf0b1 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurface.mm @@ -10,6 +10,8 @@ @interface FlutterSurface () { CGSize _size; IOSurfaceRef _ioSurface; id _texture; + // Used for testing. + BOOL _isInUseOverride; } @end @@ -27,6 +29,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..d87aed4c9e4ce 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]; @@ -266,9 +266,15 @@ - (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 () { NSMutableArray* _surfaces; + NSMapTable* _surfaceAge; } @end @@ -278,28 +284,65 @@ @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. + if (_surfaces.firstObject != nil && !CGSizeEqualToSize(_surfaces.firstObject.size, size)) { + [_surfaces removeAllObjects]; + } + + FlutterSurface* res; + + // 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 (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 || [self ageForSurface:res] > [self ageForSurface:surface])) { + 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) { + [self setAge:0 forSurface:surface]; + } + for (FlutterSurface* surface in _surfaces) { + [self setAge:[self ageForSurface:surface] + 1 forSurface:surface]; + } + + [_surfaces addObjectsFromArray:returnedSurfaces]; + + // 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 [self ageForSurface:surface] < kSurfaceEvictionAge; + }]]; } // 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..8d6996280e691 100644 --- a/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm +++ b/shell/platform/darwin/macos/framework/Source/FlutterSurfaceManagerTest.mm @@ -117,8 +117,13 @@ - (void)onPresent:(CGSize)frameSize withBlock:(nonnull dispatch_block_t)block { auto surfaceFromCache = [surfaceManager surfaceForSize:CGSizeMake(110, 110)]; EXPECT_EQ(surfaceFromCache, surface2); - [surfaceManager presentSurfaces:@[] atTime:0 notify:nil]; - EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); + // Submit empty surfaces until the one in cache gets to age >= kSurfaceEvictionAge, in which case + // it should be removed. + + 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); @@ -163,6 +168,33 @@ - (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 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); + + 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]; + // Surface in use should bet old enough at this point to be evicted. + EXPECT_EQ(surfaceManager.backBufferCache.count, 1ul); +} + inline bool operator==(const CGRect& lhs, const CGRect& rhs) { return CGRectEqualToRect(lhs, rhs); }