From abe236edb6f695780af173cafe30ea7ee4418868 Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 08:49:06 +0700 Subject: [PATCH 1/4] refactor: explicit `Arc::clone` --- crates/tarball/src/lib.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index c81755abc..d5e054b83 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -127,17 +127,17 @@ pub async fn download_tarball_to_store( let cache_value = cache_lock.write().await; match &*cache_value { CacheValue::Available(cas_paths) => { - return Ok(cas_paths.clone()); + return Ok(Arc::clone(cas_paths)); } CacheValue::InProgress(existing_notify) => { - notify = existing_notify.clone(); + notify = Arc::clone(existing_notify); } } } notify.notified().await; if let Some(cached) = cache.get(package_url) { if let CacheValue::Available(cas_paths) = &*cached.read().await { - return Ok(cas_paths.clone()); + return Ok(Arc::clone(cas_paths)); } } Err(TarballError::Io(std::io::Error::new( @@ -146,8 +146,8 @@ pub async fn download_tarball_to_store( ))) } else { let notify = Arc::new(Notify::new()); - let cache_lock = Arc::new(RwLock::new(CacheValue::InProgress(notify.clone()))); - cache.insert(package_url.to_string(), cache_lock.clone()); + let cache_lock = Arc::new(RwLock::new(CacheValue::InProgress(Arc::clone(¬ify)))); + cache.insert(package_url.to_string(), Arc::clone(&cache_lock)); let cas_paths = download_tarball_to_store_uncached( package_url, http_client, @@ -157,7 +157,7 @@ pub async fn download_tarball_to_store( ) .await?; let mut cache_write = cache_lock.write().await; - *cache_write = CacheValue::Available(cas_paths.clone()); + *cache_write = CacheValue::Available(Arc::clone(&cas_paths)); notify.notify_waiters(); Ok(cas_paths) } From baf01ee92e539085dc2679ea329186f252f6dbac Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 08:53:44 +0700 Subject: [PATCH 2/4] docs: guide regarding `Arc::clone` --- CODE_STYLE_GUIDE.md | 54 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/CODE_STYLE_GUIDE.md b/CODE_STYLE_GUIDE.md index 052c42a8e..5108546eb 100644 --- a/CODE_STYLE_GUIDE.md +++ b/CODE_STYLE_GUIDE.md @@ -174,3 +174,57 @@ assert_eq!(received, 5); ``` If the assertion fails, the value of `received` will appear alongside the error message. + +### Cloning an atomic counter + +Prefer using `Arc::clone` or `Rc::clone` to vague `.clone()` or `Clone::clone`. + +**Error resistance:** Explicitly specifying the cloned type would avoid accidentally cloning the wrong type. As seen below: + +```rust +fn my_function(value: Arc>) { + // ... do many things here + let value_clone = value.clone(); // inexpensive clone + tokio::task::spawn(async { + // ... do stuff with value_clone + }) +} +``` + +The above function could easily refactored into the following code: + +```rust +fn my_function(value: &Vec) { + // ... do many things here + let value_clone = value.clone(); // expensive clone, oops + tokio::task::spawn(async { + // ... do stuff with value_clone + }) +} +``` + +With an explicit `Arc::clone`, however, the performance characteristic will never be missed: + +```rust +fn my_function(value: Arc>) { + // ... do many things here + let value_clone = Arc::clone(&value); // no compile error + tokio::task::spawn(async { + // ... do stuff with value_clone + }) +} +``` + +```rust +fn my_function(value: &Vec) { + // ... do many things here + let value_clone = Arc::clone(&value); // compile error + tokio::task::spawn(async { + // ... do stuff with value_clone + }) +} +``` + +The above code is still valid code, and the Rust compiler doesn't error, but it has a different performance characteristic now. + +**Readability:** The generic `.clone()` or `Clone::clone` often implies an expensive operation (for example: cloning a `Vec`), but `Arc` and `Rc` are not as expensive as the generic `.clone()`. Explicitly mark the cloned type would aid in future refactoring. From c63b5289d2c81db0a434649dcb51fb82d2c52cb3 Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 09:01:03 +0700 Subject: [PATCH 3/4] refactor: reduce indentation level --- crates/tarball/src/lib.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index d5e054b83..9d63da7a2 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -122,18 +122,12 @@ pub async fn download_tarball_to_store( package_url: &str, ) -> Result>, TarballError> { if let Some(cache_lock) = cache.get(package_url) { - let notify; - { - let cache_value = cache_lock.write().await; - match &*cache_value { - CacheValue::Available(cas_paths) => { - return Ok(Arc::clone(cas_paths)); - } - CacheValue::InProgress(existing_notify) => { - notify = Arc::clone(existing_notify); - } + let notify = match &*cache_lock.write().await { + CacheValue::Available(cas_paths) => { + return Ok(Arc::clone(cas_paths)); } - } + CacheValue::InProgress(notify) => Arc::clone(notify), + }; notify.notified().await; if let Some(cached) = cache.get(package_url) { if let CacheValue::Available(cas_paths) = &*cached.read().await { From 27d23766f533ee62443c376e0ec5dcde8e494f88 Mon Sep 17 00:00:00 2001 From: khai96_ Date: Wed, 20 Sep 2023 09:03:53 +0700 Subject: [PATCH 4/4] refactor: use pipe --- crates/tarball/src/lib.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/tarball/src/lib.rs b/crates/tarball/src/lib.rs index 9d63da7a2..3219de225 100644 --- a/crates/tarball/src/lib.rs +++ b/crates/tarball/src/lib.rs @@ -140,7 +140,11 @@ pub async fn download_tarball_to_store( ))) } else { let notify = Arc::new(Notify::new()); - let cache_lock = Arc::new(RwLock::new(CacheValue::InProgress(Arc::clone(¬ify)))); + let cache_lock = notify + .pipe_ref(Arc::clone) + .pipe(CacheValue::InProgress) + .pipe(RwLock::new) + .pipe(Arc::new); cache.insert(package_url.to_string(), Arc::clone(&cache_lock)); let cas_paths = download_tarball_to_store_uncached( package_url,