oconnor663 / duct.rs

a Rust library for running child processes
MIT License
795 stars 34 forks source link

use Vec<OsString> in macro to avoid unnecessary allocation #100

Closed nui closed 2 years ago

nui commented 2 years ago

When &[OsString] is converted to Iterator, it yields item of type &OsString which will be converted to OsString again in duct::cmd.

Type of args in this macro should be Vec<OsString> so we avoid n - 1 additional allocations.

I'm not sure this is worth or not because this code shouldn't be in hot loop anyway.

oconnor663 commented 2 years ago

Good point! I can't remember; it's possible I noticed this before and couldn't figure out how to do any better. But I agree that your fix is an improvement. The Vec here is guaranteed to be pretty small, while the strings might conceivably be large and expensive to copy.

But then you inadvertently sent me down the rabbit hole of [T; N]::IntoIterator... I think we could actually get rid of the Vec with something like this...

#[macro_export]
macro_rules! cmd {
    ( $program:expr $(,)? ) => {
        {
            $crate::cmd($program, std::iter::empty::<std::ffi::OsString>())
        }
    };
    ( $program:expr $(, $arg:expr )+ $(,)? ) => {
        {
            let args = [$( Into::<std::ffi::OsString>::into($arg) ),*];
            $crate::cmd($program, IntoIterator::into_iter(args))
        }
    };
}

The first clause becomes necessary because rustc can't infer the type of the array when it's empty. And the junk with IntoIterator::into_iter is a workaroud for that ambiguity around arrays and .into_iter() to do with Edition 2021. What a mouthful.

I have no intention of committing a gross macro like that into this crate, when the performance benefit is so trivial. I'm much happier to use yours :)

nui commented 2 years ago

But then you inadvertently sent me down the rabbit hole of [T; N]::IntoIterator

I did that too, but then I realized that [T; N]::IntoIterator only return T when edition = 2021.

edit: You are right, [T; N]::IntoIterator is an exception :).