Skip to content

Commit de17042

Browse files
[Core ML] Improve asset management (#13560)
## Fix asset management issues in CoreML delegate - Do not use the temporary directory for asset storage. - Use the staging directory for storing temporary assets, and ensure it is cleaned up after use. - Perform aggressive cleanup of the temporary directory to avoid leftovers. - Guarantee proper cleanup of staging directories after operations to prevent stale files. --------- Co-authored-by: Scott Roy <[email protected]>
1 parent ed8cbc0 commit de17042

File tree

4 files changed

+215
-127
lines changed

4 files changed

+215
-127
lines changed

backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.h

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,17 @@ NS_ASSUME_NONNULL_BEGIN
9999
- (NSUInteger)compact:(NSUInteger)sizeInBytes error:(NSError* __autoreleasing*)error;
100100

101101

102+
/// Executes a block with a unique temporary directory.
103+
///
104+
/// A new temporary subdirectory URL is created inside the receiver’s designated
105+
/// base directory. The directory is passed to the block, which can use it to
106+
/// perform temporary file operations. After the block finishes executing,
107+
/// the directory and its contents are removed.
108+
///
109+
/// @param block A block to execute. The block receives a unique URL.
110+
- (void)withTemporaryDirectory:(void (^)(NSURL* directoryURL))block;
111+
112+
102113
/// Purges the assets storage. The assets are moved to the trash directory and are asynchronously
103114
/// deleted.
104115
///
@@ -117,6 +128,12 @@ NS_ASSUME_NONNULL_BEGIN
117128
/// contents are deleted asynchronously.
118129
@property (copy, readonly, nonatomic) NSURL* trashDirectoryURL;
119130

131+
132+
/// The staging directory URL, used to hold assets that are being prepared or processed
133+
/// before they are moved into their final location. The contents of this directory
134+
/// are temporary and may be cleared when no longer needed.
135+
@property (copy, readonly, nonatomic) NSURL* stagingDirectoryURL;
136+
120137
/// The file manager.
121138
@property (strong, readonly, nonatomic) NSFileManager* fileManager;
122139

backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm

Lines changed: 66 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -254,6 +254,29 @@ BOOL is_asset_alive(NSMapTable<NSString *, ETCoreMLAsset *> *assets_in_use_map,
254254

255255
return assets;
256256
}
257+
258+
NSURL * _Nullable move_to_directory(NSURL *url,
259+
NSURL *directoryURL,
260+
NSFileManager *fileManager,
261+
NSError * __autoreleasing *error) {
262+
if (!url) {
263+
ETCoreMLLogErrorAndSetNSError(error, ETCoreMLErrorInternalError, "Move operation failed: source URL is nil.");
264+
return nil;
265+
}
266+
267+
if (!directoryURL) {
268+
ETCoreMLLogErrorAndSetNSError(error, ETCoreMLErrorInternalError, "Move operation failed: destination URL is nil.");
269+
return nil;
270+
}
271+
272+
NSURL *dstURL = [directoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString];
273+
if (![fileManager moveItemAtURL:url toURL:dstURL error:error]) {
274+
return nil;
275+
}
276+
277+
return dstURL;
278+
}
279+
257280
} //namespace
258281

259282
@interface ETCoreMLAssetManager () <NSFileManagerDelegate> {
@@ -299,12 +322,17 @@ - (nullable instancetype)initWithDatabase:(const std::shared_ptr<Database>&)data
299322
if (!managedAssetsDirectoryURL) {
300323
return nil;
301324
}
302-
325+
303326
NSURL *managedTrashDirectoryURL = ::create_directory_if_needed(trashDirectoryURL, @"models", fileManager, error);
304327
if (!managedTrashDirectoryURL) {
305328
return nil;
306329
}
307-
330+
331+
NSURL *managedStagingDirectoryURL = ::create_directory_if_needed(assetsDirectoryURL, @"staging", fileManager, error);
332+
if (!managedStagingDirectoryURL) {
333+
return nil;
334+
}
335+
308336
// If directory is empty then purge the stores
309337
if (::is_directory_empty(managedAssetsDirectoryURL, fileManager, nil)) {
310338
assetsMetaStore.impl()->purge(ec);
@@ -315,6 +343,7 @@ - (nullable instancetype)initWithDatabase:(const std::shared_ptr<Database>&)data
315343
_assetsStore = std::move(assetsStore);
316344
_assetsMetaStore = std::move(assetsMetaStore);
317345
_assetsDirectoryURL = managedAssetsDirectoryURL;
346+
_stagingDirectoryURL = managedStagingDirectoryURL;
318347
_trashDirectoryURL = managedTrashDirectoryURL;
319348
_estimatedSizeInBytes = sizeInBytes.value();
320349
_maxAssetsSizeInBytes = maxAssetsSizeInBytes;
@@ -346,15 +375,15 @@ - (nullable instancetype)initWithDatabaseURL:(NSURL *)databaseURL
346375
error:error];
347376
}
348377

349-
- (nullable NSURL *)moveURL:(NSURL *)url
350-
toUniqueURLInDirectory:(NSURL *)directoryURL
351-
error:(NSError * __autoreleasing *)error {
352-
NSURL *dstURL = [directoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString];
353-
if (![self.fileManager moveItemAtURL:url toURL:dstURL error:error]) {
354-
return nil;
378+
- (void)withTemporaryDirectory:(void (^)(NSURL *directoryURL))block {
379+
NSURL *dstURL = [self.stagingDirectoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString];
380+
block(dstURL);
381+
if (![self.fileManager fileExistsAtPath:dstURL.path]) {
382+
return;
355383
}
356-
357-
return dstURL;
384+
385+
move_to_directory(dstURL, self.trashDirectoryURL, self.fileManager, nil);
386+
[self cleanupTrashDirectory];
358387
}
359388

360389
- (void)cleanupAssetIfNeeded:(ETCoreMLAsset *)asset {
@@ -407,9 +436,8 @@ - (nullable ETCoreMLAsset *)_storeAssetAtURL:(NSURL *)srcURL
407436
return false;
408437
}
409438

410-
// If an asset exists move it
411-
[self moveURL:dstURL toUniqueURLInDirectory:self.trashDirectoryURL error:nil];
412-
439+
// If a file already exists at `dstURL`, move it to the trash for removal.
440+
move_to_directory(dstURL, self.trashDirectoryURL, self.fileManager, nil);
413441
// Move the asset to assets directory.
414442
if (![self.fileManager moveItemAtURL:srcURL toURL:dstURL error:error]) {
415443
return false;
@@ -433,16 +461,25 @@ - (nullable ETCoreMLAsset *)_storeAssetAtURL:(NSURL *)srcURL
433461
}
434462

435463
- (void)triggerCompaction {
436-
if (self.estimatedSizeInBytes < self.maxAssetsSizeInBytes) {
437-
return;
464+
if (self.estimatedSizeInBytes >= self.maxAssetsSizeInBytes) {
465+
__weak __typeof(self) weakSelf = self;
466+
dispatch_async(self.syncQueue, ^{
467+
NSError *localError = nil;
468+
if (![weakSelf _compact:self.maxAssetsSizeInBytes error:&localError]) {
469+
ETCoreMLLogError(localError, "Failed to compact asset store.");
470+
}
471+
});
438472
}
439-
473+
474+
// Always clean the trash directory to ensure a minimal footprint.
475+
// The `trashQueue` is serialized, so only one cleanup will run at a time.
476+
[self cleanupTrashDirectory];
477+
}
478+
479+
- (void)cleanupTrashDirectory {
440480
__weak __typeof(self) weakSelf = self;
441-
dispatch_async(self.syncQueue, ^{
442-
NSError *localError = nil;
443-
if (![weakSelf _compact:self.maxAssetsSizeInBytes error:&localError]) {
444-
ETCoreMLLogError(localError, "Failed to compact asset store.");
445-
}
481+
dispatch_async(self.trashQueue, ^{
482+
[weakSelf removeFilesInTrashDirectory];
446483
});
447484
}
448485

@@ -548,7 +585,7 @@ - (BOOL)_removeAssetWithIdentifier:(NSString *)identifier
548585

549586
NSURL *assetURL = ::get_asset_url(assetValue);
550587
if ([self.fileManager fileExistsAtPath:assetURL.path] &&
551-
![self moveURL:assetURL toUniqueURLInDirectory:self.trashDirectoryURL error:error]) {
588+
!move_to_directory(assetURL, self.trashDirectoryURL, self.fileManager, error)) {
552589
return false;
553590
}
554591

@@ -649,13 +686,7 @@ - (NSUInteger)_compact:(NSUInteger)sizeInBytes error:(NSError * __autoreleasing
649686
identifier);
650687
}
651688
}
652-
653-
// Trigger cleanup.
654-
__weak __typeof(self) weakSelf = self;
655-
dispatch_async(self.trashQueue, ^{
656-
[weakSelf removeFilesInTrashDirectory];
657-
});
658-
689+
659690
return _estimatedSizeInBytes;
660691
}
661692

@@ -664,7 +695,10 @@ - (NSUInteger)compact:(NSUInteger)sizeInBytes error:(NSError * __autoreleasing *
664695
dispatch_sync(self.syncQueue, ^{
665696
result = [self _compact:sizeInBytes error:error];
666697
});
667-
698+
699+
// Always clean the trash directory to ensure a minimal footprint.
700+
// The `trashQueue` is serialized, so only one cleanup will run at a time.
701+
[self cleanupTrashDirectory];
668702
return result;
669703
}
670704

@@ -708,7 +742,7 @@ - (BOOL)_purge:(NSError * __autoreleasing *)error {
708742
}
709743

710744
// Move the the whole assets directory to the temp directory.
711-
if (![self moveURL:self.assetsDirectoryURL toUniqueURLInDirectory:self.trashDirectoryURL error:error]) {
745+
if (!move_to_directory(self.assetsDirectoryURL, self.trashDirectoryURL, self.fileManager, error)) {
712746
return false;
713747
}
714748

@@ -724,13 +758,7 @@ - (BOOL)_purge:(NSError * __autoreleasing *)error {
724758

725759
::set_error_from_error_code(ec, error);
726760
// Trigger cleanup
727-
if (status) {
728-
__weak __typeof(self) weakSelf = self;
729-
dispatch_async(self.trashQueue, ^{
730-
[weakSelf removeFilesInTrashDirectory];
731-
});
732-
}
733-
761+
[self cleanupTrashDirectory];
734762
return static_cast<BOOL>(status);
735763
}
736764

backends/apple/coreml/runtime/delegate/ETCoreMLModelLoader.mm

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -62,21 +62,12 @@ + (nullable ETCoreMLModel *)loadModelWithContentsOfURL:(NSURL *)compiledModelURL
6262
if (model) {
6363
return model;
6464
}
65-
66-
if (localError) {
67-
ETCoreMLLogError(localError,
68-
"Failed to load model from compiled asset with identifier = %@",
69-
identifier);
70-
}
71-
72-
// If store failed then we will load the model from compiledURL.
73-
auto backingAsset = Asset::make(compiledModelURL, identifier, assetManager.fileManager, error);
74-
if (!backingAsset) {
75-
return nil;
65+
66+
if (error) {
67+
*error = localError;
7668
}
77-
78-
asset = [[ETCoreMLAsset alloc] initWithBackingAsset:backingAsset.value()];
79-
return ::get_model_from_asset(asset, configuration, metadata, error);
69+
70+
return nil;
8071
}
8172

8273
@end

0 commit comments

Comments
 (0)