From 3ca0e642e1b38d914566bf560c1dc8f736b595bf Mon Sep 17 00:00:00 2001 From: Scott Roy Date: Wed, 17 Sep 2025 11:50:32 -0700 Subject: [PATCH] Back out "Improve asset management" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Original commit changeset: 46d2f763787b Original Phabricator Diff: D80715432 Since the introduction of D80715432, WhatsApp is seeing an increase in model load failures with error code 0x23 (P1948468840). From logs, the underlying issue is something like "Model init failed “model.mlmodelc” couldn’t be moved to “models” because an item with the same name already exists." FB app has also seen an increase in error code 0x23 since 9/5. We are unable to reproduce this error locally, but out of caution we want to revert D80715432 and see if the errors resolve. 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 --- .../runtime/delegate/ETCoreMLAssetManager.h | 17 -- .../runtime/delegate/ETCoreMLAssetManager.mm | 104 ++++----- .../runtime/delegate/ETCoreMLModelLoader.mm | 19 +- .../runtime/delegate/ETCoreMLModelManager.mm | 202 +++++++----------- 4 files changed, 127 insertions(+), 215 deletions(-) diff --git a/backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.h b/backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.h index a9e06efa90d..11d957044e9 100644 --- a/backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.h +++ b/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; diff --git a/backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm b/backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm index 53c3d1cdc69..256026e1f09 100644 --- a/backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm +++ b/backends/apple/coreml/runtime/delegate/ETCoreMLAssetManager.mm @@ -254,29 +254,6 @@ BOOL is_asset_alive(NSMapTable *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 () { @@ -322,17 +299,12 @@ - (nullable instancetype)initWithDatabase:(const std::shared_ptr&)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&)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(status); } diff --git a/backends/apple/coreml/runtime/delegate/ETCoreMLModelLoader.mm b/backends/apple/coreml/runtime/delegate/ETCoreMLModelLoader.mm index 9e8ae04842e..05aa910d954 100644 --- a/backends/apple/coreml/runtime/delegate/ETCoreMLModelLoader.mm +++ b/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 diff --git a/backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.mm b/backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.mm index c27b42566dc..f4cfd2146ac 100644 --- a/backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.mm +++ b/backends/apple/coreml/runtime/delegate/ETCoreMLModelManager.mm @@ -345,10 +345,6 @@ void add_compute_unit(std::string& identifier, MLComputeUnits compute_units) { return [ETCoreMLModelDebugInfo modelDebugInfoFromData:file_data error:error]; } -NSString *raw_model_identifier(NSString *identifier) { - return [NSString stringWithFormat:@"raw_%@", identifier]; -} - #endif } //namespace @@ -412,7 +408,7 @@ - (nullable ETCoreMLAsset *)assetWithIdentifier:(NSString *)identifier { return modelAsset; } - __block NSError *localError = nil; + NSError *localError = nil; modelAsset = [self.assetManager assetWithIdentifier:identifier error:&localError]; if (localError) { ETCoreMLLogError(localError, @@ -424,9 +420,8 @@ - (nullable ETCoreMLAsset *)assetWithIdentifier:(NSString *)identifier { } - (nullable NSURL *)compiledModelURLWithIdentifier:(NSString *)identifier - modelURL:(nullable NSURL *)modelURL inMemoryFS:(const inmemoryfs::InMemoryFileSystem*)inMemoryFS - dstURL:(NSURL *)dstURL + assetManager:(ETCoreMLAssetManager *)assetManager error:(NSError * __autoreleasing *)error { auto modelAssetType = get_model_asset_type(inMemoryFS); if (!modelAssetType) { @@ -435,132 +430,78 @@ - (nullable NSURL *)compiledModelURLWithIdentifier:(NSString *)identifier "AOT blob is missing model file."); return nil; } - - // If modelURL is not provided, write model files to the destination directory (dstURL) - // and obtain a URL pointing to them. Otherwise, use the provided modelURL. - modelURL = (modelURL == nil) ? ::write_model_files(dstURL, self.fileManager, identifier, modelAssetType.value(), inMemoryFS, error) : modelURL; - if (!modelURL) { - // Failed to generate or locate model files, return nil. - return nil; - } - - // Handle based on the type of the model asset. + + NSURL *dstURL = [self.assetManager.trashDirectoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString]; + NSURL *modelURL = ::write_model_files(dstURL, self.fileManager, identifier, modelAssetType.value(), inMemoryFS, error); switch (modelAssetType.value()) { case ModelAssetType::CompiledModel: { - // The model is already compiled; no further action needed. - // Return the existing model URL. + // Model is already compiled. return modelURL; } - + case ModelAssetType::Model: { - // The model is not compiled yet. - // Compile the model at the specified URL with a maximum wait time of 5 minutes. + // Compile the model. NSURL *compiledModelURL = [ETCoreMLModelCompiler compileModelAtURL:modelURL maxWaitTimeInSeconds:(5 * 60) error:error]; - // Return the URL of the compiled model or nil if compilation fails. + return compiledModelURL; } } } -- (nullable ETCoreMLAsset *)compiledModelAssetWithMetadata:(const ModelMetadata&)metadata - modelURL:(nullable NSURL *)modelURL - inMemoryFS:(const inmemoryfs::InMemoryFileSystem*)inMemoryFS - error:(NSError * __autoreleasing *)error { - NSString *identifier = @(metadata.identifier.c_str()); - __block ETCoreMLAsset *compiledModelAsset = [self assetWithIdentifier:identifier]; - if (compiledModelAsset) { - ETCoreMLLogInfo("Cache Hit: Successfully retrieved compiled model with identifier=%@ from the models cache.", identifier); - } else { - ETCoreMLLogInfo("Cache Miss: Compiled Model with identifier=%@ was not found in the models cache.", identifier); - } - - [self.assetManager withTemporaryDirectory:^(NSURL * _Nonnull directoryURL) { - if (compiledModelAsset) { - return; - } - - // The directory specified by `directoryURL` is unique and will be automatically cleaned up - // once the enclosing block completes. - NSURL *compiledModelURL = [self compiledModelURLWithIdentifier:identifier - modelURL:modelURL - inMemoryFS:inMemoryFS - dstURL:directoryURL - error:error]; - if (compiledModelURL) { - // Move the compiled model to the asset manager to transfer ownership. - compiledModelAsset = [self.assetManager storeAssetAtURL:compiledModelURL withIdentifier:identifier error:error]; - } - }]; - - return compiledModelAsset; -} - #if ET_EVENT_TRACER_ENABLED -- (nullable ETCoreMLAsset *)modelAssetWithMetadata:(const ModelMetadata&)metadata - inMemoryFS:(const inmemoryfs::InMemoryFileSystem*)inMemoryFS - error:(NSError * __autoreleasing *)error { +- (nullable id)modelExecutorWithMetadata:(const ModelMetadata&)metadata + inMemoryFS:(const inmemoryfs::InMemoryFileSystem*)inMemoryFS + configuration:(MLModelConfiguration *)configuration + error:(NSError * __autoreleasing *)error { NSString *identifier = @(metadata.identifier.c_str()); - NSString *rawIdentifier = raw_model_identifier(identifier); - __block ETCoreMLAsset *modelAsset = [self assetWithIdentifier:rawIdentifier]; - if (modelAsset) { + // Otherwise try to retrieve the compiled asset. + ETCoreMLAsset *compiledModelAsset = [self assetWithIdentifier:identifier]; + if (compiledModelAsset) { ETCoreMLLogInfo("Cache Hit: Successfully retrieved model with identifier=%@ from the models cache.", identifier); } else { ETCoreMLLogInfo("Cache Miss: Model with identifier=%@ was not found in the models cache.", identifier); } - - [self.assetManager withTemporaryDirectory:^(NSURL * _Nonnull directoryURL) { - if (modelAsset) { - return; - } - - auto modelAssetType = get_model_asset_type(inMemoryFS); - if (modelAssetType != ModelAssetType::Model) { - return; - } - - // The directory specified by `directoryURL` is unique and will be automatically cleaned up - // once the enclosing block completes. - NSURL *modelURL = ::write_model_files(directoryURL, - self.fileManager, - identifier, - modelAssetType.value(), - inMemoryFS, - error); + + // Create a unique directory for writing model files. + NSURL *dstURL = [self.assetManager.trashDirectoryURL URLByAppendingPathComponent:[NSUUID UUID].UUIDString]; + auto modelAssetType = get_model_asset_type(inMemoryFS); + ETCoreMLAsset *modelAsset = nil; + // Write the model files. + if (modelAssetType == ModelAssetType::Model) { + NSURL *modelURL = ::write_model_files(dstURL, self.fileManager, identifier, modelAssetType.value(), inMemoryFS, error); if (modelURL) { - // Move the model to the asset manager to transfer ownership. - modelAsset = [self.assetManager storeAssetAtURL:modelURL withIdentifier:rawIdentifier error:error]; + modelAsset = make_asset(modelURL, + identifier, + self.fileManager, + error); } - }]; - - return modelAsset; -} - -- (nullable id)modelExecutorWithMetadata:(const ModelMetadata&)metadata - inMemoryFS:(const inmemoryfs::InMemoryFileSystem*)inMemoryFS - configuration:(MLModelConfiguration *)configuration - error:(NSError * __autoreleasing *)error { - NSError *localError = nil; - ETCoreMLAsset *modelAsset = [self modelAssetWithMetadata:metadata inMemoryFS:inMemoryFS error:&localError]; - if (localError) { - if (error) { - *error = localError; - } - - return nil; } - - ETCoreMLAsset *compiledModelAsset = [self compiledModelAssetWithMetadata:metadata - modelURL:modelAsset.contentURL - inMemoryFS:inMemoryFS - error:error]; + + if (!compiledModelAsset) { + // Compile the model. + NSURL *compiledModelURL = [self compiledModelURLWithIdentifier:identifier + inMemoryFS:inMemoryFS + assetManager:self.assetManager + error:error]; + compiledModelAsset = make_asset(compiledModelURL, + identifier, + self.fileManager, + error); + } + if (!compiledModelAsset) { return nil; } + + NSError *localError = nil; + ETCoreMLModelDebugInfo *debug_info = get_model_debug_info(inMemoryFS, &localError); + if (localError) { + ETCoreMLLogError(localError, "Failed to parse debug info file"); + } + - ETCoreMLModelDebugInfo *debug_info = get_model_debug_info(inMemoryFS, error); - // The analyzer requires both the raw (uncompiled) asset and the compiled model asset to perform analysis. return [[ETCoreMLModelAnalyzer alloc] initWithCompiledModelAsset:compiledModelAsset modelAsset:modelAsset modelDebugInfo:debug_info @@ -569,33 +510,41 @@ - (nullable ETCoreMLAsset *)modelAssetWithMetadata:(const ModelMetadata&)metadat assetManager:self.assetManager error:error]; } + #else - (nullable id)modelExecutorWithMetadata:(const ModelMetadata&)metadata inMemoryFS:(const inmemoryfs::InMemoryFileSystem*)inMemoryFS configuration:(MLModelConfiguration *)configuration error:(NSError * __autoreleasing *)error { - ETCoreMLAsset *compiledModelAsset = [self compiledModelAssetWithMetadata:metadata - modelURL:nil - inMemoryFS:inMemoryFS - error:error]; - if (!compiledModelAsset) { - return nil; + NSString *identifier = @(metadata.identifier.c_str()); + // Otherwise try to retrieve the compiled asset. + ETCoreMLAsset *asset = [self assetWithIdentifier:identifier]; + ETCoreMLModel *model = asset ? get_model_from_asset(asset, configuration, metadata, error) : nil; + if (model) { + ETCoreMLLogInfo("Cache Hit: Successfully retrieved model with identifier=%@ from the models cache.", identifier); + return [[ETCoreMLDefaultModelExecutor alloc] initWithModel:model]; } - - ETCoreMLModel *model = [ETCoreMLModelLoader loadModelWithContentsOfURL:compiledModelAsset.contentURL - configuration:configuration - metadata:metadata - assetManager:self.assetManager - error:error]; - if (!model) { + + ETCoreMLLogInfo("Cache Miss: Model with identifier=%@ was not found in the models cache.", identifier); + // Compile the model. + NSURL *compiledModelURL = [self compiledModelURLWithIdentifier:identifier + inMemoryFS:inMemoryFS + assetManager:self.assetManager + error:error]; + if (!compiledModelURL) { return nil; } - + + model = [ETCoreMLModelLoader loadModelWithContentsOfURL:compiledModelURL + configuration:configuration + metadata:metadata + assetManager:self.assetManager + error:error]; + return [[ETCoreMLDefaultModelExecutor alloc] initWithModel:model]; } #endif - - (nullable id)_modelExecutorWithAOTData:(NSData *)data configuration:(MLModelConfiguration *)configuration error:(NSError * __autoreleasing *)error { @@ -780,7 +729,6 @@ - (BOOL)executeModelWithHandle:(ModelHandle *)handle args.count); return result; } - NSError *localError = nil; @autoreleasepool { NSArray *inputs = [args subarrayWithRange:NSMakeRange(0, model.orderedInputNames.count)]; @@ -800,11 +748,11 @@ - (BOOL)executeModelWithHandle:(ModelHandle *)handle result = YES; } } - - if (localError && error) { - *error = localError; + if (!result) { + if (error) { + *error = localError; + } } - return result; }