Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Question on optimizing our use of Arcs #2239

Open
thunderbiscuit opened this issue Sep 19, 2024 · 1 comment
Open

Question on optimizing our use of Arcs #2239

thunderbiscuit opened this issue Sep 19, 2024 · 1 comment

Comments

@thunderbiscuit
Copy link

thunderbiscuit commented Sep 19, 2024

We have this type (simplified for this issue):

pub struct TxOut {
    pub value: Arc<Amount>,
    pub number: u32,
}

Which we need to move to and from its Rust equivalent (which we type alias as RustTxOut):

impl From<TxOut> for RustTxOut {
    fn from(tx_out: TxOut) -> Self {
        let value = match Arc::try_unwrap(tx_out.value) {
            Ok(val) => val.0,
            Err(arc) => arc.0.clone(),
        };

        RustTxOut {
            value,
            script_pubkey,
        }
    }
}

A dev proposed this Arc::try_unwrap() optimization to remove the requirement for the clone if the Arc counter is at 1. This looks good, but once I attempted to clear my understanding of the exact behaviour here through some testing, I'm left wondering if the Arc can ever be 1 and this optimization ever hit the Ok case. See my issue here for full details.

The gist of it is that after testing Kotlin using the following:

  1. First I added this method on one of my types that would trigger this "consuption of the Arc":
pub fn use_tx_out(&self, tx_out: TxOut) -> String {
    println!("Reference count before try_unwrap: {}", Arc::strong_count(&tx_out.value));
    let rust_tx_out: BitcoinTxOut = tx_out.into();
    "Your TxOut has been consumed".to_string()
}

And then used it in a test:

val output1: LocalOutput = wallet.listOutput().first()
val message = wallet.useTxOut(output1.txout)
println("Message: $message")
println("TxOut is: ${output1.txout.value.toSat()}")

The printed output is:

Reference count before try_unwrap: 2

ArcTest > testArcs() STANDARD_OUT
    Balance: 2024456
    Message: Your TxOut has been consumed
    TxOut is: 4200

So the test succeeds in printing the TxOut after consuming it in a method and we know why in this case: the reference count is 2. My question is now... why is the count at 2 here? ChatGPT seems to think it might be that uniffi holds an extra reference somewhere for you, but you know what they say about ChatGPT; NEVER TRUST THE MOFO WITH PRODUCTION CODE.

My question is then:

  1. can someone help me clarify this for myself? Does uniffi hold an inner reference to all my complex types anyway and this type of optimization will never actually trigger because both my Kotlin code and uniffi will have a reference?
  2. What do you think of these type of optimizations? Do they play into uniffi's turf too much and should be avoided? Or is that something that makes total sense and is a good idea to adopt widely in our codebase?
@linabutler
Copy link
Member

linabutler commented Sep 19, 2024

Hi @thunderbiscuit!

  1. Yep, that's correct! If you take a look at the generated scaffolding code (try running cargo expand and searching for unsafe impl ::uniffi::FfiConverterArc<crate::UniFfiTag> for Amount), you'll see this comment:
/// When writing as a field of a complex structure, make a clone and transfer ownership
/// of it to the foreign-language code by writing its pointer into the buffer.
/// The foreign-language code is responsible for freeing this by calling the
/// `ffi_object_free` FFI function provided by the corresponding UniFFI type.

So your analysis is spot on: the first strong reference is the one that you created here, and the second strong reference is the one that UniFFI created for passing the Amount to Kotlin code. Kotlin is a garbage-collected language, so while that second strong reference will be freed by the Cleaner "eventually", there's no guarantees about when exactly that will happen.

Did you happen to try Swift, by chance? Swift's deinitializers have more predictable semantics, specifically when used with withExtendedLifetime; I'm curious if your unwrap-or-clone optimization works there!

  1. I don't think it hurts anything to leave that optimization in, though (and Rust 1.76 introduced Arc::unwrap_or_clone, which does exactly what you're doing now! 🎉).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants