From c70916b2ca52805f1fcf53bdf30bafba56cbb6b8 Mon Sep 17 00:00:00 2001 From: olaszakos Date: Wed, 22 May 2024 12:26:50 +0200 Subject: [PATCH 1/8] upload command uses same codebase as sync --- src/canisters/frontend/ic-asset/src/lib.rs | 4 +- src/canisters/frontend/ic-asset/src/sync.rs | 37 ++++++++-- src/canisters/frontend/ic-asset/src/upload.rs | 74 ------------------- .../frontend/icx-asset/src/commands/sync.rs | 8 +- .../frontend/icx-asset/src/commands/upload.rs | 63 +++------------- src/canisters/frontend/icx-asset/src/main.rs | 4 +- src/dfx/src/lib/installers/assets/mod.rs | 43 +++++++---- 7 files changed, 79 insertions(+), 154 deletions(-) delete mode 100644 src/canisters/frontend/ic-asset/src/upload.rs diff --git a/src/canisters/frontend/ic-asset/src/lib.rs b/src/canisters/frontend/ic-asset/src/lib.rs index 9e9fa6f454..0b31f9d8aa 100644 --- a/src/canisters/frontend/ic-asset/src/lib.rs +++ b/src/canisters/frontend/ic-asset/src/lib.rs @@ -37,9 +37,7 @@ mod canister_api; pub mod error; mod evidence; mod sync; -mod upload; pub use evidence::compute_evidence; pub use sync::prepare_sync_for_proposal; -pub use sync::sync; -pub use upload::upload; +pub use sync::{sync, ExistingAssetStrategy}; diff --git a/src/canisters/frontend/ic-asset/src/sync.rs b/src/canisters/frontend/ic-asset/src/sync.rs index d0e145239c..288d4047b5 100644 --- a/src/canisters/frontend/ic-asset/src/sync.rs +++ b/src/canisters/frontend/ic-asset/src/sync.rs @@ -40,10 +40,21 @@ use walkdir::WalkDir; const KNOWN_DIRECTORIES: [&str; 1] = [".well-known"]; +/// Strategy for handling assets in the canister that are not in the new asset set. +#[derive(Debug)] +pub enum ExistingAssetStrategy { + /// Keep assets that are not in the new asset set. This will add to the destination canister without removing anything. + Keep, + + /// Delete assets that are not in the new asset set. This will keep the destination canister in sync with the source directories. + Delete, +} + /// Sets the contents of the asset canister to the contents of a directory, including deleting old assets. pub async fn upload_content_and_assemble_sync_operations( canister: &Canister<'_>, dirs: &[&Path], + existing_asset_strategy: ExistingAssetStrategy, logger: &Logger, ) -> Result { let asset_descriptors = gather_asset_descriptors(dirs, logger)?; @@ -77,7 +88,10 @@ pub async fn upload_content_and_assemble_sync_operations( let commit_batch_args = batch_upload::operations::assemble_commit_batch_arguments( project_assets, canister_assets, - AssetDeletionReason::Obsolete, + match existing_asset_strategy { + ExistingAssetStrategy::Keep => AssetDeletionReason::Incompatible, + ExistingAssetStrategy::Delete => AssetDeletionReason::Obsolete, + }, canister_asset_properties, batch_id, ); @@ -105,10 +119,16 @@ pub async fn upload_content_and_assemble_sync_operations( pub async fn sync( canister: &Canister<'_>, dirs: &[&Path], + existing_asset_strategy: ExistingAssetStrategy, logger: &Logger, ) -> Result<(), SyncError> { - let commit_batch_args = - upload_content_and_assemble_sync_operations(canister, dirs, logger).await?; + let commit_batch_args = upload_content_and_assemble_sync_operations( + canister, + dirs, + existing_asset_strategy, + logger, + ) + .await?; let canister_api_version = api_version(canister).await; debug!(logger, "Canister API version: {canister_api_version}. ic-asset API version: {BATCH_UPLOAD_API_VERSION}"); info!(logger, "Committing batch."); @@ -140,7 +160,7 @@ async fn commit_in_stages( commit_batch( canister, CommitBatchArguments { - batch_id: Nat::from(0_u8), + batch_id: commit_batch_args.batch_id.clone(), operations: operations.into(), }, ) @@ -180,9 +200,16 @@ async fn commit_in_stages( pub async fn prepare_sync_for_proposal( canister: &Canister<'_>, dirs: &[&Path], + existing_asset_strategy: ExistingAssetStrategy, logger: &Logger, ) -> Result<(), PrepareSyncForProposalError> { - let arg = upload_content_and_assemble_sync_operations(canister, dirs, logger).await?; + let arg = upload_content_and_assemble_sync_operations( + canister, + dirs, + existing_asset_strategy, + logger, + ) + .await?; let arg = sort_batch_operations(arg); let batch_id = arg.batch_id.clone(); diff --git a/src/canisters/frontend/ic-asset/src/upload.rs b/src/canisters/frontend/ic-asset/src/upload.rs deleted file mode 100644 index 7057a9f4b1..0000000000 --- a/src/canisters/frontend/ic-asset/src/upload.rs +++ /dev/null @@ -1,74 +0,0 @@ -use crate::asset::config::AssetConfig; -use crate::batch_upload::operations::BATCH_UPLOAD_API_VERSION; -use crate::batch_upload::{ - self, - operations::AssetDeletionReason, - plumbing::{make_project_assets, AssetDescriptor, ChunkUploader}, -}; -use crate::canister_api::methods::{ - api_version::api_version, - batch::{commit_batch, create_batch}, - list::list_assets, -}; -use crate::canister_api::types::batch_upload::v0; -use crate::error::CompatibilityError::DowngradeV1TOV0Failed; -use crate::error::UploadError; -use crate::error::UploadError::{CommitBatchFailed, CreateBatchFailed, ListAssetsFailed}; -use ic_utils::Canister; -use slog::{info, Logger}; -use std::collections::HashMap; -use std::path::PathBuf; - -/// Upload the specified files -pub async fn upload( - canister: &Canister<'_>, - files: HashMap, - logger: &Logger, -) -> Result<(), UploadError> { - let asset_descriptors: Vec = files - .iter() - .map(|x| AssetDescriptor { - source: x.1.clone(), - key: x.0.clone(), - config: AssetConfig::default(), - }) - .collect(); - - let canister_assets = list_assets(canister).await.map_err(ListAssetsFailed)?; - - info!(logger, "Starting batch."); - - let batch_id = create_batch(canister).await.map_err(CreateBatchFailed)?; - - info!(logger, "Staging contents of new and changed assets:"); - - let chunk_upload_target = ChunkUploader::new(canister.clone(), batch_id.clone()); - - let project_assets = make_project_assets( - Some(&chunk_upload_target), - asset_descriptors, - &canister_assets, - logger, - ) - .await?; - - let commit_batch_args = batch_upload::operations::assemble_commit_batch_arguments( - project_assets, - canister_assets, - AssetDeletionReason::Incompatible, - HashMap::new(), - batch_id, - ); - - let canister_api_version = api_version(canister).await; - info!(logger, "Committing batch."); - match canister_api_version { - 0 => { - let commit_batch_args_v0 = v0::CommitBatchArguments::try_from(commit_batch_args) - .map_err(DowngradeV1TOV0Failed)?; - commit_batch(canister, commit_batch_args_v0).await - } - BATCH_UPLOAD_API_VERSION.. => commit_batch(canister, commit_batch_args).await, - } - .map_err(CommitBatchFailed) -} diff --git a/src/canisters/frontend/icx-asset/src/commands/sync.rs b/src/canisters/frontend/icx-asset/src/commands/sync.rs index 3718664922..9ea357d333 100644 --- a/src/canisters/frontend/icx-asset/src/commands/sync.rs +++ b/src/canisters/frontend/icx-asset/src/commands/sync.rs @@ -9,6 +9,12 @@ pub(crate) async fn sync( logger: &Logger, ) -> anyhow::Result<()> { let dirs: Vec<&Path> = o.directory.iter().map(|d| d.as_path()).collect(); - ic_asset::sync(canister, &dirs, logger).await?; + ic_asset::sync( + canister, + &dirs, + ic_asset::ExistingAssetStrategy::Delete, + logger, + ) + .await?; Ok(()) } diff --git a/src/canisters/frontend/icx-asset/src/commands/upload.rs b/src/canisters/frontend/icx-asset/src/commands/upload.rs index e7f974299a..54f97162bf 100644 --- a/src/canisters/frontend/icx-asset/src/commands/upload.rs +++ b/src/canisters/frontend/icx-asset/src/commands/upload.rs @@ -1,63 +1,20 @@ use crate::UploadOpts; use ic_utils::Canister; use slog::Logger; -use std::collections::HashMap; -use std::path::PathBuf; -use std::str::FromStr; -use walkdir::WalkDir; +use std::path::Path; pub(crate) async fn upload( canister: &Canister<'_>, - opts: &UploadOpts, + o: &UploadOpts, logger: &Logger, ) -> anyhow::Result<()> { - let key_map = get_key_map(&opts.files)?; - ic_asset::upload(canister, key_map, logger).await?; + let dirs: Vec<&Path> = o.directory.iter().map(|d| d.as_path()).collect(); + ic_asset::sync( + canister, + &dirs, + ic_asset::ExistingAssetStrategy::Keep, + logger, + ) + .await?; Ok(()) } - -fn get_key_map(files: &[String]) -> anyhow::Result> { - let mut key_map: HashMap = HashMap::new(); - - for arg in files { - let (key, source): (String, PathBuf) = { - if let Some(index) = arg.find('=') { - ( - arg[..index].to_string(), - PathBuf::from_str(&arg[index + 1..])?, - ) - } else { - let source = PathBuf::from_str(&arg.clone())?; - let key = format!("/{}", source.file_name().unwrap().to_string_lossy()); - // or if we want to retain relative paths: - // let key = if source.is_absolute() { - // format!("/{}", source.file_name().unwrap().to_string_lossy()) - // } else { - // format!("/{}", arg.clone()) - // }; - (key, source) - } - }; - - if source.is_file() { - key_map.insert(key, source); - } else { - for p in WalkDir::new(source.clone()) - .into_iter() - .filter_map(std::result::Result::ok) - .filter(|e| !e.file_type().is_dir()) - { - let p = p.path().to_path_buf(); - let relative = p.strip_prefix(&source).expect("cannot strip prefix"); - let mut key = key.clone(); - if !key.ends_with('/') { - key.push('/'); - } - key.push_str(relative.to_string_lossy().as_ref()); - key_map.insert(key, p); - } - } - } - - Ok(key_map) -} diff --git a/src/canisters/frontend/icx-asset/src/main.rs b/src/canisters/frontend/icx-asset/src/main.rs index 818180c6dd..25481a8a4e 100644 --- a/src/canisters/frontend/icx-asset/src/main.rs +++ b/src/canisters/frontend/icx-asset/src/main.rs @@ -72,8 +72,8 @@ struct UploadOpts { /// The asset canister ID to manage. canister_id: String, - /// Files or folders to send. - files: Vec, + /// Directories to upload. + directory: Vec, } fn create_identity(maybe_pem: Option) -> Box { diff --git a/src/dfx/src/lib/installers/assets/mod.rs b/src/dfx/src/lib/installers/assets/mod.rs index 9614f63b00..21e33b2a02 100644 --- a/src/dfx/src/lib/installers/assets/mod.rs +++ b/src/dfx/src/lib/installers/assets/mod.rs @@ -4,6 +4,7 @@ use crate::lib::error::DfxResult; use anyhow::Context; use fn_error_context::context; use ic_agent::Agent; +use ic_asset::ExistingAssetStrategy; use slog::Logger; use std::path::Path; @@ -27,14 +28,19 @@ pub async fn post_install_store_assets( .build() .context("Failed to build asset canister caller.")?; - ic_asset::sync(&canister, &source_paths, logger) - .await - .with_context(|| { - format!( - "Failed asset sync with canister {}.", - canister.canister_id_() - ) - })?; + ic_asset::sync( + &canister, + &source_paths, + ExistingAssetStrategy::Delete, + logger, + ) + .await + .with_context(|| { + format!( + "Failed asset sync with canister {}.", + canister.canister_id_() + ) + })?; Ok(()) } @@ -59,14 +65,19 @@ pub async fn prepare_assets_for_proposal( .build() .context("Failed to build asset canister caller.")?; - ic_asset::prepare_sync_for_proposal(&canister, &source_paths, logger) - .await - .with_context(|| { - format!( - "Failed asset sync with canister {}.", - canister.canister_id_() - ) - })?; + ic_asset::prepare_sync_for_proposal( + &canister, + &source_paths, + ExistingAssetStrategy::Delete, + logger, + ) + .await + .with_context(|| { + format!( + "Failed asset sync with canister {}.", + canister.canister_id_() + ) + })?; Ok(()) } From c44a122f427ef02a7f3919eca5a642d3e4fa567a Mon Sep 17 00:00:00 2001 From: olaszakos Date: Wed, 22 May 2024 12:26:58 +0200 Subject: [PATCH 2/8] increase chunk creation parallelism --- .../frontend/ic-asset/src/batch_upload/semaphores.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/batch_upload/semaphores.rs b/src/canisters/frontend/ic-asset/src/batch_upload/semaphores.rs index 835aedd2e4..1f86278586 100644 --- a/src/canisters/frontend/ic-asset/src/batch_upload/semaphores.rs +++ b/src/canisters/frontend/ic-asset/src/batch_upload/semaphores.rs @@ -4,13 +4,13 @@ use futures_intrusive::sync::SharedSemaphore; const MAX_SIMULTANEOUS_LOADED_MB: usize = 50; // How many simultaneous chunks being created at once -const MAX_SIMULTANEOUS_CREATE_CHUNK: usize = 12; +const MAX_SIMULTANEOUS_CREATE_CHUNK: usize = 50; // How many simultaneous Agent.call() to create_chunk -const MAX_SIMULTANEOUS_CREATE_CHUNK_CALLS: usize = 4; +const MAX_SIMULTANEOUS_CREATE_CHUNK_CALLS: usize = 25; // How many simultaneous Agent.wait() on create_chunk result -const MAX_SIMULTANEOUS_CREATE_CHUNK_WAITS: usize = 4; +const MAX_SIMULTANEOUS_CREATE_CHUNK_WAITS: usize = 25; pub(crate) struct Semaphores { // The "file" semaphore limits how much file data to load at once. A given loaded file's data From d0b8e0fe73112597898d8a32055055054ce97e7d Mon Sep 17 00:00:00 2001 From: olaszakos Date: Wed, 22 May 2024 12:28:02 +0200 Subject: [PATCH 3/8] parallelize asset properties query --- .../canister_api/methods/asset_properties.rs | 24 +++++++++++++++---- src/canisters/frontend/ic-asset/src/sync.rs | 7 ++++++ 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/src/canisters/frontend/ic-asset/src/canister_api/methods/asset_properties.rs b/src/canisters/frontend/ic-asset/src/canister_api/methods/asset_properties.rs index 066432b249..23280f81bb 100644 --- a/src/canisters/frontend/ic-asset/src/canister_api/methods/asset_properties.rs +++ b/src/canisters/frontend/ic-asset/src/canister_api/methods/asset_properties.rs @@ -4,20 +4,36 @@ use crate::canister_api::{ }; use crate::error::GetAssetPropertiesError; use crate::error::GetAssetPropertiesError::GetAssetPropertiesFailed; +use futures_intrusive::sync::SharedSemaphore; use ic_agent::{agent::RejectResponse, AgentError}; use ic_utils::call::SyncCall; use ic_utils::Canister; use std::collections::HashMap; +const MAX_CONCURRENT_REQUESTS: usize = 20; + pub(crate) async fn get_assets_properties( canister: &Canister<'_>, canister_assets: &HashMap, ) -> Result, GetAssetPropertiesError> { + let semaphore = SharedSemaphore::new(true, MAX_CONCURRENT_REQUESTS); + + let asset_ids = canister_assets.keys().cloned().collect::>(); + let futs = asset_ids + .iter() + .map(|asset_id| async { + semaphore.acquire(1).await; + get_asset_properties(canister, asset_id).await + }) + .collect::>(); + + let results = futures::future::join_all(futs).await; + let mut all_assets_properties = HashMap::new(); - for asset_id in canister_assets.keys() { - match get_asset_properties(canister, asset_id).await { + for (index, result) in results.into_iter().enumerate() { + match result { Ok(asset_properties) => { - all_assets_properties.insert(asset_id.to_string(), asset_properties); + all_assets_properties.insert(asset_ids[index].to_string(), asset_properties); } // older canisters don't have get_assets_properties method // therefore we can break the loop @@ -29,7 +45,7 @@ pub(crate) async fn get_assets_properties( break; } Err(e) => { - return Err(GetAssetPropertiesFailed(asset_id.clone(), e)); + return Err(GetAssetPropertiesFailed(asset_ids[index].clone(), e)); } } } diff --git a/src/canisters/frontend/ic-asset/src/sync.rs b/src/canisters/frontend/ic-asset/src/sync.rs index 288d4047b5..7e9639c9a0 100644 --- a/src/canisters/frontend/ic-asset/src/sync.rs +++ b/src/canisters/frontend/ic-asset/src/sync.rs @@ -64,8 +64,15 @@ pub async fn upload_content_and_assemble_sync_operations( logger, "Fetching properties for all assets in the canister." ); + let now = std::time::Instant::now(); let canister_asset_properties = get_assets_properties(canister, &canister_assets).await?; + info!( + logger, + "Done fetching properties for all assets in the canister. Took {:?}", + now.elapsed() + ); + info!(logger, "Starting batch."); let batch_id = create_batch(canister).await.map_err(CreateBatchFailed)?; From cce6ae0a2aeaee015612d49ce93047342e9fb4ec Mon Sep 17 00:00:00 2001 From: olaszakos Date: Wed, 22 May 2024 12:28:19 +0200 Subject: [PATCH 4/8] support log levels --- src/canisters/frontend/icx-asset/src/main.rs | 29 +++++++++++++++++-- .../frontend/icx-asset/src/support.rs | 5 ++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/src/canisters/frontend/icx-asset/src/main.rs b/src/canisters/frontend/icx-asset/src/main.rs index 25481a8a4e..c052f5750d 100644 --- a/src/canisters/frontend/icx-asset/src/main.rs +++ b/src/canisters/frontend/icx-asset/src/main.rs @@ -6,9 +6,10 @@ use crate::commands::upload::upload; use anstyle::{AnsiColor, Style}; use candid::Principal; use clap::builder::Styles; -use clap::{crate_authors, crate_version, Parser}; +use clap::{crate_authors, crate_version, Parser, ValueEnum}; use ic_agent::identity::{AnonymousIdentity, BasicIdentity, Secp256k1Identity}; use ic_agent::{agent, Agent, Identity}; +use slog::Level; use std::path::PathBuf; const DEFAULT_IC_GATEWAY: &str = "https://icp0.io"; @@ -37,6 +38,30 @@ struct Opts { #[command(subcommand)] subcommand: SubCommand, + + #[arg(long, value_enum, default_value = "info")] + log_level: LogLevel, +} + +#[derive(ValueEnum, Clone, Debug)] +enum LogLevel { + Trace, + Debug, + Info, + Warning, + Error, +} + +impl From for Level { + fn from(log_level: LogLevel) -> Self { + match log_level { + LogLevel::Trace => Level::Trace, + LogLevel::Debug => Level::Debug, + LogLevel::Info => Level::Info, + LogLevel::Warning => Level::Warning, + LogLevel::Error => Level::Error, + } + } } #[derive(Parser)] @@ -107,7 +132,7 @@ fn style() -> Styles { async fn main() -> anyhow::Result<()> { let opts: Opts = Opts::parse(); - let logger = support::new_logger(); + let logger = support::new_logger(opts.log_level.into()); let agent = Agent::builder() .with_transport(agent::http_transport::ReqwestTransport::create( diff --git a/src/canisters/frontend/icx-asset/src/support.rs b/src/canisters/frontend/icx-asset/src/support.rs index 298bfdfc91..0cf5c897c7 100644 --- a/src/canisters/frontend/icx-asset/src/support.rs +++ b/src/canisters/frontend/icx-asset/src/support.rs @@ -1,4 +1,4 @@ -use slog::{Drain, Logger}; +use slog::{Drain, Level, Logger}; pub struct TermLogFormat where @@ -42,9 +42,10 @@ impl slog::Drain for TermLogFormat { } } -pub(crate) fn new_logger() -> Logger { +pub(crate) fn new_logger(level: Level) -> Logger { let decorator = slog_term::TermDecorator::new().build(); let drain = TermLogFormat::new(decorator).fuse(); + let drain = slog::LevelFilter::new(drain, level).fuse(); let drain = slog_async::Async::new(drain).build().fuse(); Logger::root(drain, slog::o!()) } From 96d47825c4c340241fb2154c5f0648087fb8a355 Mon Sep 17 00:00:00 2001 From: olaszakos Date: Wed, 22 May 2024 16:18:12 +0200 Subject: [PATCH 5/8] fix code example --- src/canisters/frontend/ic-asset/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/canisters/frontend/ic-asset/src/lib.rs b/src/canisters/frontend/ic-asset/src/lib.rs index 0b31f9d8aa..87179cd9c8 100644 --- a/src/canisters/frontend/ic-asset/src/lib.rs +++ b/src/canisters/frontend/ic-asset/src/lib.rs @@ -20,7 +20,7 @@ //! .with_agent(&agent) //! .build()?; //! let logger = slog::Logger::root(slog::Discard, slog::o!()); -//! ic_asset::sync(&canister, &[concat!(env!("CARGO_MANIFEST_DIR"), "assets/").as_ref()], &logger).await?; +//! ic_asset::sync(&canister, &[concat!(env!("CARGO_MANIFEST_DIR"), "assets/").as_ref()], ic_asset::ExistingAssetStrategy::Delete, &logger).await?; //! # Ok(()) //! # } From 14b14c6fb049caf0dfec5a364709ad0f562353d3 Mon Sep 17 00:00:00 2001 From: olaszakos Date: Thu, 23 May 2024 11:13:08 +0200 Subject: [PATCH 6/8] Revert "upload command uses same codebase as sync" This reverts commit c70916b2ca52805f1fcf53bdf30bafba56cbb6b8. --- src/canisters/frontend/ic-asset/src/lib.rs | 4 +- src/canisters/frontend/ic-asset/src/sync.rs | 37 ++-------- src/canisters/frontend/ic-asset/src/upload.rs | 74 +++++++++++++++++++ .../frontend/icx-asset/src/commands/sync.rs | 8 +- .../frontend/icx-asset/src/commands/upload.rs | 63 +++++++++++++--- src/canisters/frontend/icx-asset/src/main.rs | 4 +- src/dfx/src/lib/installers/assets/mod.rs | 43 ++++------- 7 files changed, 154 insertions(+), 79 deletions(-) create mode 100644 src/canisters/frontend/ic-asset/src/upload.rs diff --git a/src/canisters/frontend/ic-asset/src/lib.rs b/src/canisters/frontend/ic-asset/src/lib.rs index 87179cd9c8..4ff4675beb 100644 --- a/src/canisters/frontend/ic-asset/src/lib.rs +++ b/src/canisters/frontend/ic-asset/src/lib.rs @@ -37,7 +37,9 @@ mod canister_api; pub mod error; mod evidence; mod sync; +mod upload; pub use evidence::compute_evidence; pub use sync::prepare_sync_for_proposal; -pub use sync::{sync, ExistingAssetStrategy}; +pub use sync::sync; +pub use upload::upload; diff --git a/src/canisters/frontend/ic-asset/src/sync.rs b/src/canisters/frontend/ic-asset/src/sync.rs index 7e9639c9a0..e7a303229b 100644 --- a/src/canisters/frontend/ic-asset/src/sync.rs +++ b/src/canisters/frontend/ic-asset/src/sync.rs @@ -40,21 +40,10 @@ use walkdir::WalkDir; const KNOWN_DIRECTORIES: [&str; 1] = [".well-known"]; -/// Strategy for handling assets in the canister that are not in the new asset set. -#[derive(Debug)] -pub enum ExistingAssetStrategy { - /// Keep assets that are not in the new asset set. This will add to the destination canister without removing anything. - Keep, - - /// Delete assets that are not in the new asset set. This will keep the destination canister in sync with the source directories. - Delete, -} - /// Sets the contents of the asset canister to the contents of a directory, including deleting old assets. pub async fn upload_content_and_assemble_sync_operations( canister: &Canister<'_>, dirs: &[&Path], - existing_asset_strategy: ExistingAssetStrategy, logger: &Logger, ) -> Result { let asset_descriptors = gather_asset_descriptors(dirs, logger)?; @@ -95,10 +84,7 @@ pub async fn upload_content_and_assemble_sync_operations( let commit_batch_args = batch_upload::operations::assemble_commit_batch_arguments( project_assets, canister_assets, - match existing_asset_strategy { - ExistingAssetStrategy::Keep => AssetDeletionReason::Incompatible, - ExistingAssetStrategy::Delete => AssetDeletionReason::Obsolete, - }, + AssetDeletionReason::Obsolete, canister_asset_properties, batch_id, ); @@ -126,16 +112,10 @@ pub async fn upload_content_and_assemble_sync_operations( pub async fn sync( canister: &Canister<'_>, dirs: &[&Path], - existing_asset_strategy: ExistingAssetStrategy, logger: &Logger, ) -> Result<(), SyncError> { - let commit_batch_args = upload_content_and_assemble_sync_operations( - canister, - dirs, - existing_asset_strategy, - logger, - ) - .await?; + let commit_batch_args = + upload_content_and_assemble_sync_operations(canister, dirs, logger).await?; let canister_api_version = api_version(canister).await; debug!(logger, "Canister API version: {canister_api_version}. ic-asset API version: {BATCH_UPLOAD_API_VERSION}"); info!(logger, "Committing batch."); @@ -167,7 +147,7 @@ async fn commit_in_stages( commit_batch( canister, CommitBatchArguments { - batch_id: commit_batch_args.batch_id.clone(), + batch_id: Nat::from(0_u8), operations: operations.into(), }, ) @@ -207,16 +187,9 @@ async fn commit_in_stages( pub async fn prepare_sync_for_proposal( canister: &Canister<'_>, dirs: &[&Path], - existing_asset_strategy: ExistingAssetStrategy, logger: &Logger, ) -> Result<(), PrepareSyncForProposalError> { - let arg = upload_content_and_assemble_sync_operations( - canister, - dirs, - existing_asset_strategy, - logger, - ) - .await?; + let arg = upload_content_and_assemble_sync_operations(canister, dirs, logger).await?; let arg = sort_batch_operations(arg); let batch_id = arg.batch_id.clone(); diff --git a/src/canisters/frontend/ic-asset/src/upload.rs b/src/canisters/frontend/ic-asset/src/upload.rs new file mode 100644 index 0000000000..7057a9f4b1 --- /dev/null +++ b/src/canisters/frontend/ic-asset/src/upload.rs @@ -0,0 +1,74 @@ +use crate::asset::config::AssetConfig; +use crate::batch_upload::operations::BATCH_UPLOAD_API_VERSION; +use crate::batch_upload::{ + self, + operations::AssetDeletionReason, + plumbing::{make_project_assets, AssetDescriptor, ChunkUploader}, +}; +use crate::canister_api::methods::{ + api_version::api_version, + batch::{commit_batch, create_batch}, + list::list_assets, +}; +use crate::canister_api::types::batch_upload::v0; +use crate::error::CompatibilityError::DowngradeV1TOV0Failed; +use crate::error::UploadError; +use crate::error::UploadError::{CommitBatchFailed, CreateBatchFailed, ListAssetsFailed}; +use ic_utils::Canister; +use slog::{info, Logger}; +use std::collections::HashMap; +use std::path::PathBuf; + +/// Upload the specified files +pub async fn upload( + canister: &Canister<'_>, + files: HashMap, + logger: &Logger, +) -> Result<(), UploadError> { + let asset_descriptors: Vec = files + .iter() + .map(|x| AssetDescriptor { + source: x.1.clone(), + key: x.0.clone(), + config: AssetConfig::default(), + }) + .collect(); + + let canister_assets = list_assets(canister).await.map_err(ListAssetsFailed)?; + + info!(logger, "Starting batch."); + + let batch_id = create_batch(canister).await.map_err(CreateBatchFailed)?; + + info!(logger, "Staging contents of new and changed assets:"); + + let chunk_upload_target = ChunkUploader::new(canister.clone(), batch_id.clone()); + + let project_assets = make_project_assets( + Some(&chunk_upload_target), + asset_descriptors, + &canister_assets, + logger, + ) + .await?; + + let commit_batch_args = batch_upload::operations::assemble_commit_batch_arguments( + project_assets, + canister_assets, + AssetDeletionReason::Incompatible, + HashMap::new(), + batch_id, + ); + + let canister_api_version = api_version(canister).await; + info!(logger, "Committing batch."); + match canister_api_version { + 0 => { + let commit_batch_args_v0 = v0::CommitBatchArguments::try_from(commit_batch_args) + .map_err(DowngradeV1TOV0Failed)?; + commit_batch(canister, commit_batch_args_v0).await + } + BATCH_UPLOAD_API_VERSION.. => commit_batch(canister, commit_batch_args).await, + } + .map_err(CommitBatchFailed) +} diff --git a/src/canisters/frontend/icx-asset/src/commands/sync.rs b/src/canisters/frontend/icx-asset/src/commands/sync.rs index 9ea357d333..3718664922 100644 --- a/src/canisters/frontend/icx-asset/src/commands/sync.rs +++ b/src/canisters/frontend/icx-asset/src/commands/sync.rs @@ -9,12 +9,6 @@ pub(crate) async fn sync( logger: &Logger, ) -> anyhow::Result<()> { let dirs: Vec<&Path> = o.directory.iter().map(|d| d.as_path()).collect(); - ic_asset::sync( - canister, - &dirs, - ic_asset::ExistingAssetStrategy::Delete, - logger, - ) - .await?; + ic_asset::sync(canister, &dirs, logger).await?; Ok(()) } diff --git a/src/canisters/frontend/icx-asset/src/commands/upload.rs b/src/canisters/frontend/icx-asset/src/commands/upload.rs index 54f97162bf..e7f974299a 100644 --- a/src/canisters/frontend/icx-asset/src/commands/upload.rs +++ b/src/canisters/frontend/icx-asset/src/commands/upload.rs @@ -1,20 +1,63 @@ use crate::UploadOpts; use ic_utils::Canister; use slog::Logger; -use std::path::Path; +use std::collections::HashMap; +use std::path::PathBuf; +use std::str::FromStr; +use walkdir::WalkDir; pub(crate) async fn upload( canister: &Canister<'_>, - o: &UploadOpts, + opts: &UploadOpts, logger: &Logger, ) -> anyhow::Result<()> { - let dirs: Vec<&Path> = o.directory.iter().map(|d| d.as_path()).collect(); - ic_asset::sync( - canister, - &dirs, - ic_asset::ExistingAssetStrategy::Keep, - logger, - ) - .await?; + let key_map = get_key_map(&opts.files)?; + ic_asset::upload(canister, key_map, logger).await?; Ok(()) } + +fn get_key_map(files: &[String]) -> anyhow::Result> { + let mut key_map: HashMap = HashMap::new(); + + for arg in files { + let (key, source): (String, PathBuf) = { + if let Some(index) = arg.find('=') { + ( + arg[..index].to_string(), + PathBuf::from_str(&arg[index + 1..])?, + ) + } else { + let source = PathBuf::from_str(&arg.clone())?; + let key = format!("/{}", source.file_name().unwrap().to_string_lossy()); + // or if we want to retain relative paths: + // let key = if source.is_absolute() { + // format!("/{}", source.file_name().unwrap().to_string_lossy()) + // } else { + // format!("/{}", arg.clone()) + // }; + (key, source) + } + }; + + if source.is_file() { + key_map.insert(key, source); + } else { + for p in WalkDir::new(source.clone()) + .into_iter() + .filter_map(std::result::Result::ok) + .filter(|e| !e.file_type().is_dir()) + { + let p = p.path().to_path_buf(); + let relative = p.strip_prefix(&source).expect("cannot strip prefix"); + let mut key = key.clone(); + if !key.ends_with('/') { + key.push('/'); + } + key.push_str(relative.to_string_lossy().as_ref()); + key_map.insert(key, p); + } + } + } + + Ok(key_map) +} diff --git a/src/canisters/frontend/icx-asset/src/main.rs b/src/canisters/frontend/icx-asset/src/main.rs index c052f5750d..46bde91c29 100644 --- a/src/canisters/frontend/icx-asset/src/main.rs +++ b/src/canisters/frontend/icx-asset/src/main.rs @@ -97,8 +97,8 @@ struct UploadOpts { /// The asset canister ID to manage. canister_id: String, - /// Directories to upload. - directory: Vec, + /// Files or folders to send. + files: Vec, } fn create_identity(maybe_pem: Option) -> Box { diff --git a/src/dfx/src/lib/installers/assets/mod.rs b/src/dfx/src/lib/installers/assets/mod.rs index 21e33b2a02..9614f63b00 100644 --- a/src/dfx/src/lib/installers/assets/mod.rs +++ b/src/dfx/src/lib/installers/assets/mod.rs @@ -4,7 +4,6 @@ use crate::lib::error::DfxResult; use anyhow::Context; use fn_error_context::context; use ic_agent::Agent; -use ic_asset::ExistingAssetStrategy; use slog::Logger; use std::path::Path; @@ -28,19 +27,14 @@ pub async fn post_install_store_assets( .build() .context("Failed to build asset canister caller.")?; - ic_asset::sync( - &canister, - &source_paths, - ExistingAssetStrategy::Delete, - logger, - ) - .await - .with_context(|| { - format!( - "Failed asset sync with canister {}.", - canister.canister_id_() - ) - })?; + ic_asset::sync(&canister, &source_paths, logger) + .await + .with_context(|| { + format!( + "Failed asset sync with canister {}.", + canister.canister_id_() + ) + })?; Ok(()) } @@ -65,19 +59,14 @@ pub async fn prepare_assets_for_proposal( .build() .context("Failed to build asset canister caller.")?; - ic_asset::prepare_sync_for_proposal( - &canister, - &source_paths, - ExistingAssetStrategy::Delete, - logger, - ) - .await - .with_context(|| { - format!( - "Failed asset sync with canister {}.", - canister.canister_id_() - ) - })?; + ic_asset::prepare_sync_for_proposal(&canister, &source_paths, logger) + .await + .with_context(|| { + format!( + "Failed asset sync with canister {}.", + canister.canister_id_() + ) + })?; Ok(()) } From 2e2957eca00365cf839b886dcff15324612c3c14 Mon Sep 17 00:00:00 2001 From: olaszakos Date: Thu, 23 May 2024 11:13:23 +0200 Subject: [PATCH 7/8] Revert "fix code example" This reverts commit 96d47825c4c340241fb2154c5f0648087fb8a355. --- src/canisters/frontend/ic-asset/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/canisters/frontend/ic-asset/src/lib.rs b/src/canisters/frontend/ic-asset/src/lib.rs index 4ff4675beb..9e9fa6f454 100644 --- a/src/canisters/frontend/ic-asset/src/lib.rs +++ b/src/canisters/frontend/ic-asset/src/lib.rs @@ -20,7 +20,7 @@ //! .with_agent(&agent) //! .build()?; //! let logger = slog::Logger::root(slog::Discard, slog::o!()); -//! ic_asset::sync(&canister, &[concat!(env!("CARGO_MANIFEST_DIR"), "assets/").as_ref()], ic_asset::ExistingAssetStrategy::Delete, &logger).await?; +//! ic_asset::sync(&canister, &[concat!(env!("CARGO_MANIFEST_DIR"), "assets/").as_ref()], &logger).await?; //! # Ok(()) //! # } From a758de5523bace24ac3d589a295c42a9cc5e3c27 Mon Sep 17 00:00:00 2001 From: olaszakos Date: Mon, 27 May 2024 08:15:39 +0200 Subject: [PATCH 8/8] update changelog --- CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 40c9ab340a..b19b0bafdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ # UNRELEASED +### perf: improve sync command performance + +Improves `sync` (eg. `dfx deploy`, `icx-asset sync`) performance by parallelization: +- Make asset properties query faster by parallelization, significant improvement for canisters that have many assets +- Make chunk creation process faster, by increasing parallelization 4=>25, significant improvement when deploying lots of small assets + +`icx-asset`: add support for log levels, defaulting to `info` + ### PocketIC support Passing `--pocketic` to `dfx start` now starts a PocketIC server instead of the replica. PocketIC is lighter-weight than the replica and execution environment internals can be manipulated by REST commands. For more information, see the [PocketIC readme](https://github.com/dfinity/pocketic).