Back out "Improve asset management" (#14569)

Summary:
Original commit changeset: 46d2f763787b

Original Phabricator Diff: D80715432

Since D80715432 landed, we also landed D81256207 to add more logging. To
prevent conflicts, we backout in 3 steps:
* In diff 1/3, we backout D81256207, which added more logging
* In diff 2/3, we backout D80715432, which is the problematic diff we're
trying to revert
* In diff 3/3, we introduce some of the logging we backed out in diff
1/3

Differential Revision: D82581443

Co-authored-by: Scott Roy <[email protected]>
Co-authored-by: Gregory Comer <[email protected]>

Author: 3 people

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

@@ -99,17 +99,6 @@ NS_ASSUME_NONNULL_BEGIN
 - (NSUInteger)compact:(NSUInteger)sizeInBytes error:(NSError* __autoreleasing*)error;
 
 
-/// Executes a block with a unique temporary directory.
-///
-/// A new temporary subdirectory URL is created inside the receiver’s designated
-/// base directory. The directory is passed to the block, which can use it to
-/// perform temporary file operations. After the block finishes executing,
-/// the directory and its contents are removed.
-///
-/// @param block A block to execute. The block receives a unique URL.
-- (void)withTemporaryDirectory:(void (^)(NSURL* directoryURL))block;
-
-
 /// Purges the assets storage. The assets are moved to the trash directory and are asynchronously
 /// deleted.
 ///
@@ -128,12 +117,6 @@ NS_ASSUME_NONNULL_BEGIN
 /// contents are deleted asynchronously.
 @property (copy, readonly, nonatomic) NSURL* trashDirectoryURL;
 
-
-/// The staging directory URL, used to hold assets that are being prepared or processed
-/// before they are moved into their final location. The contents of this directory
-/// are temporary and may be cleared when no longer needed.
-@property (copy, readonly, nonatomic) NSURL* stagingDirectoryURL;
-
 /// The file manager.
 @property (strong, readonly, nonatomic) NSFileManager* fileManager;
 

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

@@ -254,29 +254,6 @@ BOOL is_asset_alive(NSMapTable<NSString *, ETCoreMLAsset *> *assets_in_use_map,
 
     return assets;
 }
-
-NSURL * _Nullable move_to_directory(NSURL *url,
-                                    NSURL *directoryURL,
-                                    NSFileManager *fileManager,
-                                    NSError * __autoreleasing *error) {
-    if (!url) {
-        ETCoreMLLogErrorAndSetNSError(error, ETCoreMLErrorInternalError, "Move operation failed: source URL is nil.");
-        return nil;
-    }
-
-    if (!directoryURL) {
-        ETCoreMLLogErrorAndSetNSError(error, ETCoreMLErrorInternalError, "Move operation failed: destination URL is nil.");
-        return nil;
-    }
-
-    NSURL *dstURL = [directoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString];
-    if (![fileManager moveItemAtURL:url toURL:dstURL error:error]) {
-        return nil;
-    }
-
-    return dstURL;
-}
-
 } //namespace
 
 @interface ETCoreMLAssetManager () <NSFileManagerDelegate> {
@@ -322,17 +299,12 @@ - (nullable instancetype)initWithDatabase:(const std::shared_ptr<Database>&)data
     if (!managedAssetsDirectoryURL) {
         return nil;
     }
-
+    
     NSURL *managedTrashDirectoryURL = ::create_directory_if_needed(trashDirectoryURL, @"models", fileManager, error);
     if (!managedTrashDirectoryURL) {
         return nil;
     }
-
-    NSURL *managedStagingDirectoryURL = ::create_directory_if_needed(assetsDirectoryURL, @"staging", fileManager, error);
-    if (!managedStagingDirectoryURL) {
-        return nil;
-    }
-
+    
     // If directory is empty then purge the stores
     if (::is_directory_empty(managedAssetsDirectoryURL, fileManager, nil)) {
         assetsMetaStore.impl()->purge(ec);
@@ -343,7 +315,6 @@ - (nullable instancetype)initWithDatabase:(const std::shared_ptr<Database>&)data
         _assetsStore = std::move(assetsStore);
         _assetsMetaStore = std::move(assetsMetaStore);
         _assetsDirectoryURL = managedAssetsDirectoryURL;
-        _stagingDirectoryURL = managedStagingDirectoryURL;
         _trashDirectoryURL = managedTrashDirectoryURL;
         _estimatedSizeInBytes = sizeInBytes.value();
         _maxAssetsSizeInBytes = maxAssetsSizeInBytes;
@@ -375,15 +346,15 @@ - (nullable instancetype)initWithDatabaseURL:(NSURL *)databaseURL
                             error:error];
 }
 
-- (void)withTemporaryDirectory:(void (^)(NSURL *directoryURL))block {
-    NSURL *dstURL = [self.stagingDirectoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString];
-    block(dstURL);
-    if (![self.fileManager fileExistsAtPath:dstURL.path]) {
-        return;
+- (nullable NSURL *)moveURL:(NSURL *)url
+     toUniqueURLInDirectory:(NSURL *)directoryURL
+                      error:(NSError * __autoreleasing *)error {
+    NSURL *dstURL = [directoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString];
+    if (![self.fileManager moveItemAtURL:url toURL:dstURL error:error]) {
+        return nil;
     }
-
-    move_to_directory(dstURL, self.trashDirectoryURL, self.fileManager, nil);
-    [self cleanupTrashDirectory];
+    
+    return dstURL;
 }
 
 - (void)cleanupAssetIfNeeded:(ETCoreMLAsset *)asset {
@@ -436,8 +407,9 @@ - (nullable ETCoreMLAsset *)_storeAssetAtURL:(NSURL *)srcURL
             return false;
         }
 
-        // If a file already exists at `dstURL`, move it to the trash for removal.
-        move_to_directory(dstURL, self.trashDirectoryURL, self.fileManager, nil);
+        // If an asset exists move it
+        [self moveURL:dstURL toUniqueURLInDirectory:self.trashDirectoryURL error:nil];
+        
         // Move the asset to assets directory.
         if (![self.fileManager moveItemAtURL:srcURL toURL:dstURL error:error]) {
             return false;
@@ -461,25 +433,16 @@ - (nullable ETCoreMLAsset *)_storeAssetAtURL:(NSURL *)srcURL
 }
 
 - (void)triggerCompaction {
-    if (self.estimatedSizeInBytes >= self.maxAssetsSizeInBytes) {
-        __weak __typeof(self) weakSelf = self;
-        dispatch_async(self.syncQueue, ^{
-            NSError *localError = nil;
-            if (![weakSelf _compact:self.maxAssetsSizeInBytes error:&localError]) {
-                ETCoreMLLogError(localError, "Failed to compact asset store.");
-            }
-        });
+    if (self.estimatedSizeInBytes < self.maxAssetsSizeInBytes) {
+        return;
     }
-
-    // Always clean the trash directory to ensure a minimal footprint.
-    // The `trashQueue` is serialized, so only one cleanup will run at a time.
-    [self cleanupTrashDirectory];
-}
-
-- (void)cleanupTrashDirectory {
+    
     __weak __typeof(self) weakSelf = self;
-    dispatch_async(self.trashQueue, ^{
-        [weakSelf removeFilesInTrashDirectory];
+    dispatch_async(self.syncQueue, ^{
+        NSError *localError = nil;
+        if (![weakSelf _compact:self.maxAssetsSizeInBytes error:&localError]) {
+            ETCoreMLLogError(localError, "Failed to compact asset store.");
+        }
     });
 }
 
@@ -585,7 +548,7 @@ - (BOOL)_removeAssetWithIdentifier:(NSString *)identifier
 
         NSURL *assetURL = ::get_asset_url(assetValue);
         if ([self.fileManager fileExistsAtPath:assetURL.path] &&
-            !move_to_directory(assetURL, self.trashDirectoryURL, self.fileManager, error)) {
+            ![self moveURL:assetURL toUniqueURLInDirectory:self.trashDirectoryURL error:error]) {
             return false;
         }
 
@@ -686,7 +649,13 @@ - (NSUInteger)_compact:(NSUInteger)sizeInBytes error:(NSError * __autoreleasing
                              identifier);
         }
     }
-
+    
+    // Trigger cleanup.
+    __weak __typeof(self) weakSelf = self;
+    dispatch_async(self.trashQueue, ^{
+        [weakSelf removeFilesInTrashDirectory];
+    });
+    
     return _estimatedSizeInBytes;
 }
 
@@ -695,10 +664,7 @@ - (NSUInteger)compact:(NSUInteger)sizeInBytes error:(NSError * __autoreleasing *
     dispatch_sync(self.syncQueue, ^{
         result = [self _compact:sizeInBytes error:error];
     });
-
-    // Always clean the trash directory to ensure a minimal footprint.
-    // The `trashQueue` is serialized, so only one cleanup will run at a time.
-    [self cleanupTrashDirectory];
+    
     return result;
 }
 
@@ -742,7 +708,7 @@ - (BOOL)_purge:(NSError * __autoreleasing *)error {
         }
 
         // Move the the whole assets directory to the temp directory.
-        if (!move_to_directory(self.assetsDirectoryURL, self.trashDirectoryURL, self.fileManager, error)) {
+        if (![self moveURL:self.assetsDirectoryURL toUniqueURLInDirectory:self.trashDirectoryURL error:error]) {
             return false;
         }
 
@@ -758,7 +724,13 @@ - (BOOL)_purge:(NSError * __autoreleasing *)error {
 
     ::set_error_from_error_code(ec, error);
     // Trigger cleanup
-    [self cleanupTrashDirectory];
+    if (status) {
+        __weak __typeof(self) weakSelf = self;
+        dispatch_async(self.trashQueue, ^{
+            [weakSelf removeFilesInTrashDirectory];
+        });
+    }
+    
     return static_cast<BOOL>(status);
 }
 

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

@@ -62,12 +62,21 @@ + (nullable ETCoreMLModel *)loadModelWithContentsOfURL:(NSURL *)compiledModelURL
     if (model) {
         return model;
     }
-
-    if (error) {
-        *error = localError;
+    
+    if (localError) {
+        ETCoreMLLogError(localError,
+                         "Failed to load model from compiled asset with identifier = %@",
+                         identifier);
     }
-
-    return nil;
+    
+    // If store failed then we will load the model from compiledURL.
+    auto backingAsset = Asset::make(compiledModelURL, identifier, assetManager.fileManager, error);
+    if (!backingAsset) {
+        return nil;
+    }
+    
+    asset = [[ETCoreMLAsset alloc] initWithBackingAsset:backingAsset.value()];
+    return ::get_model_from_asset(asset, configuration, metadata, error);
 }
 
 @end