feat(rust): Remove unnecessary Mutex overhead for resource references#823
Open
LucaButBoring wants to merge 1 commit intosmithy-lang:main-1.xfrom
Open
feat(rust): Remove unnecessary Mutex overhead for resource references#823LucaButBoring wants to merge 1 commit intosmithy-lang:main-1.xfrom
LucaButBoring wants to merge 1 commit intosmithy-lang:main-1.xfrom
Conversation
Replace Rc<RefCell<dyn T>> with Rc<T> for resource references in generated Rust code. Since resource traits already require Send + Sync, the inner Mutex (which RefCell becomes with the sync feature) was redundant. To maintain backwards compatibility with existing code that uses .lock().unwrap() on resource references, introduce ResourceInner<T> and ResourceGuard<T> types that provide a no-op .lock() method. This allows existing code to continue working while eliminating the locking overhead. The lock() method now returns Result<ResourceGuard, Infallible> instead of Result<MutexGuard, PoisonError>. Code that explicitly handles PoisonError will need to be updated, though most code uses .unwrap() or .expect() and will continue to work unchanged. Changes: - types/resource.rs: Add ResourceInner<T> wrapper around Rc<T> with no-op lock() method, and ResourceGuard<T> for the returned guard type - conversions/resource.rs: Use ResourceInner::new() instead of Rc::new(RefCell::new())
LucaButBoring
force-pushed
the
lucalc/no-lock
branch
from
282c6c4 to
80b9cb1
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removed unnecessary locking overhead for resource references in generated Rust code.
Problem:
Generated resource types used
Rc<RefCell<dyn T>>, which becomesArc<Mutex<dyn T>>with thesyncfeature. Since resource traits already requireSend + Sync, the implementations must be internally thread-safe, making the outer Mutex redundant, and therefore making every invocation pay the locking overhead for no benefit.Solution:
We replace
Rc<RefCell<dyn T>>withRc<T>directly. To maintain backwards-compatibility with existing code that calls.lock().unwrap()on resource references, we introduce newResourceInner<T>andResourceGuard<T>types that provide a no-op.lock()method. This can be simplified if we decide this is a rare/invalid use case.There is still one minor breaking change, however:
lock()now returnsResult<ResourceGuard, Infallible>instead ofResult<MutexGuard, PoisonError>. Code that callsPoisonError-specific methods (e.g.,.into_inner()) will fail to compile - code that simply uses.unwrap(),.expect()(typical use case), or pattern matches without using the error value will continue to work. If we decide this is a problem, we can introduce a new error type that matches the same API, instead.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.