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

feature: Add new crate gix-macros #992

Merged
merged 29 commits into from
Sep 7, 2023
Merged

Conversation

NobodyXu
Copy link
Contributor

@NobodyXu NobodyXu commented Aug 20, 2023

To make de-monomorphizatioin easier without sacrification of code readability as mentioned in #987

TODO

  • momo: Put the inner function inside the momoed function for any non-self function.
    E.g.
        fn f(s: impl AsRef<str>) {
            fn inner(s: &str) {}
            inner(s.as_ref())
        }
  • Also Add tests for this on this: Self and without Self.
  • Support parsing impl block so that we can apply momo to trait implementation
  • momo: Remove unused generics in TryInto: Removed anything related to its Error
  • momo clone::PrepareFetch::new
  • momo repository/object.rs: Repository::commit_as
  • momo discover.rs
  • momo init.rs
  • momo open::repository.rs
  • Handle pattern in param name
  • momo create::into
  • (low-priority) fixed the unused mut errors: Remove all mut in the generic fn except for AsMut, and remove mut from AsMut target in inner fn. User can opt-out of this by annotating #[keep] in front of the AsMut param name.

Expanded test code

Command for getting expanded code:

cargo expand  --test test

Actual expanded tests/test.rs code:

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
use gix_macros::momo;
use std::pin::Pin;
struct Options;
#[allow(dead_code)]
fn test_open_opts_inner(
    _dir: impl Into<std::path::PathBuf>,
    _options: Options,
) -> Result<(), ()> {
    Ok(())
}
/// See if doc are kept
#[allow(dead_code)]
fn test_open_opts(
    directory: impl Into<std::path::PathBuf>,
    options: Options,
) -> Result<(), ()> {
    #[allow(unused_mut)]
    #[allow(dead_code)]
    fn _test_open_opts_inner_generated_by_gix_macro_momo(
        directory: std::path::PathBuf,
        options: Options,
    ) -> Result<(), ()> {
        test_open_opts_inner(directory, options)
    }
    _test_open_opts_inner_generated_by_gix_macro_momo(directory.into(), options)
}
#[allow(dead_code)]
fn test_with_try(
    t: impl TryInto<String, Error = ()>,
    _options: Options,
) -> Result<(), ()> {
    #[allow(unused_mut)]
    #[allow(dead_code)]
    fn _test_with_try_inner_generated_by_gix_macro_momo(
        t: String,
        _options: Options,
    ) -> Result<(), ()> {
        let t = t;
        t.strip_prefix('1').ok_or(())?;
        Ok(())
    }
    _test_with_try_inner_generated_by_gix_macro_momo(t.try_into()?, _options)
}
fn test_fn<E>(
    a: impl Into<String>,
    b: impl AsRef<str>,
    mut c: impl AsMut<str>,
    d: impl TryInto<String, Error = E>,
) -> Result<String, E> {
    #[allow(unused_mut)]
    fn _test_fn_inner_generated_by_gix_macro_momo<E>(
        a: String,
        b: &str,
        mut c: &mut str,
        d: String,
    ) -> Result<String, E> {
        let mut s = a;
        s += b;
        s += c;
        s += &d;
        Ok(s)
    }
    _test_fn_inner_generated_by_gix_macro_momo(
        a.into(),
        b.as_ref(),
        c.as_mut(),
        d.try_into()?,
    )
}
fn test_fn_call_style<E>(
    a: impl Into<String>,
    b: impl AsRef<str>,
    mut c: impl AsMut<str>,
    d: impl TryInto<String, Error = E>,
) -> Result<String, E> {
    #[allow(unused_mut)]
    fn _test_fn_call_style_inner_generated_by_gix_macro_momo<E>(
        a: String,
        b: &str,
        mut c: &mut str,
        d: String,
    ) -> Result<String, E> {
        let mut s = a;
        s += b;
        s += c;
        s += &d;
        Ok(s)
    }
    _test_fn_call_style_inner_generated_by_gix_macro_momo(
        a.into(),
        b.as_ref(),
        c.as_mut(),
        d.try_into()?,
    )
}
fn test_fn_where<A, B, C, D, E>(a: A, b: B, mut c: C, d: D) -> Result<String, E>
where
    A: Into<String>,
    B: AsRef<str>,
    C: AsMut<str>,
    D: TryInto<String, Error = E>,
{
    #[allow(unused_mut)]
    fn _test_fn_where_inner_generated_by_gix_macro_momo<E>(
        a: String,
        b: &str,
        mut c: &mut str,
        d: String,
    ) -> Result<String, E> {
        let mut s = a;
        s += b;
        s += c;
        s += &d;
        Ok(s)
    }
    _test_fn_where_inner_generated_by_gix_macro_momo(
        a.into(),
        b.as_ref(),
        c.as_mut(),
        d.try_into()?,
    )
}
struct TestStruct;
impl TestStruct {
    fn test_method<E>(
        self,
        a: impl Into<String>,
        b: impl AsRef<str>,
        mut c: impl AsMut<str>,
        d: impl TryInto<String, Error = E>,
    ) -> Result<String, E> {
        self._test_method_inner_generated_by_gix_macro_momo(
            a.into(),
            b.as_ref(),
            c.as_mut(),
            d.try_into()?,
        )
    }
    #[allow(unused_mut)]
    fn _test_method_inner_generated_by_gix_macro_momo<E>(
        self,
        a: String,
        b: &str,
        mut c: &mut str,
        d: String,
    ) -> Result<String, E> {
        let mut s = a;
        s += b;
        s += c;
        s += &d;
        Ok(s)
    }
    #[allow(clippy::needless_arbitrary_self_type)]
    fn test_method2<E>(
        self: Self,
        a: impl Into<String>,
        b: impl AsRef<str>,
        mut c: impl AsMut<str>,
        d: impl TryInto<String, Error = E>,
    ) -> Result<String, E> {
        self._test_method2_inner_generated_by_gix_macro_momo(
            a.into(),
            b.as_ref(),
            c.as_mut(),
            d.try_into()?,
        )
    }
    #[allow(unused_mut)]
    #[allow(clippy::needless_arbitrary_self_type)]
    fn _test_method2_inner_generated_by_gix_macro_momo<E>(
        self: Self,
        a: String,
        b: &str,
        mut c: &mut str,
        d: String,
    ) -> Result<String, E> {
        let mut s = a;
        s += b;
        s += c;
        s += &d;
        Ok(s)
    }
    fn test_fn<E>(
        a: impl Into<String>,
        b: impl AsRef<str>,
        mut c: impl AsMut<str>,
        d: impl TryInto<String, Error = E>,
        _e: (),
    ) -> Result<String, E> {
        #[allow(unused_mut)]
        fn _test_fn_inner_generated_by_gix_macro_momo<E>(
            a: String,
            b: &str,
            mut c: &mut str,
            d: String,
            _e: (),
        ) -> Result<String, E> {
            let mut s = a;
            s += b;
            s += c;
            s += &d;
            Ok(s)
        }
        _test_fn_inner_generated_by_gix_macro_momo(
            a.into(),
            b.as_ref(),
            c.as_mut(),
            d.try_into()?,
            _e,
        )
    }
    fn test_fn2<E>(
        _this: Self,
        a: impl Into<String>,
        b: impl AsRef<str>,
        mut c: impl AsMut<str>,
        d: impl TryInto<String, Error = E>,
        _e: (),
        _f: (),
    ) -> Result<String, E> {
        Self::_test_fn2_inner_generated_by_gix_macro_momo(
            _this,
            a.into(),
            b.as_ref(),
            c.as_mut(),
            d.try_into()?,
            _e,
            _f,
        )
    }
    #[allow(unused_mut)]
    fn _test_fn2_inner_generated_by_gix_macro_momo<E>(
        _this: Self,
        a: String,
        b: &str,
        mut c: &mut str,
        d: String,
        _e: (),
        _f: (),
    ) -> Result<String, E> {
        let mut s = a;
        s += b;
        s += c;
        s += &d;
        Ok(s)
    }
    fn test_fn3<E>(
        _this: Pin<&mut Self>,
        a: impl Into<String>,
        b: impl AsRef<str>,
        mut c: impl AsMut<str>,
        d: impl TryInto<String, Error = E>,
        _e: (),
        _f: (),
    ) -> Result<String, E> {
        Self::_test_fn3_inner_generated_by_gix_macro_momo(
            _this,
            a.into(),
            b.as_ref(),
            c.as_mut(),
            d.try_into()?,
            _e,
            _f,
        )
    }
    #[allow(unused_mut)]
    fn _test_fn3_inner_generated_by_gix_macro_momo<E>(
        _this: Pin<&mut Self>,
        a: String,
        b: &str,
        mut c: &mut str,
        d: String,
        _e: (),
        _f: (),
    ) -> Result<String, E> {
        let mut s = a;
        s += b;
        s += c;
        s += &d;
        Ok(s)
    }
}
struct S(bool);
impl TryInto<String> for S {
    type Error = ();
    fn try_into(self) -> Result<String, ()> {
        if self.0 { Ok(String::from("!2345")) } else { Err(()) }
    }
}
extern crate test;
#[cfg(test)]
#[rustc_test_marker = "test_basic_fn"]
pub const test_basic_fn: test::TestDescAndFn = test::TestDescAndFn {
    desc: test::TestDesc {
        name: test::StaticTestName("test_basic_fn"),
        ignore: false,
        ignore_message: ::core::option::Option::None,
        source_file: "gix-macros/tests/test.rs",
        start_line: 175usize,
        start_col: 4usize,
        end_line: 175usize,
        end_col: 17usize,
        compile_fail: false,
        no_run: false,
        should_panic: test::ShouldPanic::No,
        test_type: test::TestType::IntegrationTest,
    },
    testfn: test::StaticTestFn(|| test::assert_test_result(test_basic_fn())),
};
fn test_basic_fn() {
    match (
        &test_fn("12345", "12345", String::from("12345"), S(true)).unwrap(),
        &"123451234512345!2345",
    ) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
    test_fn("12345", "12345", String::from("12345"), S(false)).unwrap_err();
    match (
        &test_fn_call_style("12345", "12345", String::from("12345"), S(true)).unwrap(),
        &"123451234512345!2345",
    ) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
    test_fn_call_style("12345", "12345", String::from("12345"), S(false)).unwrap_err();
    match (
        &test_fn_where("12345", "12345", String::from("12345"), S(true)).unwrap(),
        &"123451234512345!2345",
    ) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
    test_fn_where("12345", "12345", String::from("12345"), S(false)).unwrap_err();
}
extern crate test;
#[cfg(test)]
#[rustc_test_marker = "test_struct_method"]
pub const test_struct_method: test::TestDescAndFn = test::TestDescAndFn {
    desc: test::TestDesc {
        name: test::StaticTestName("test_struct_method"),
        ignore: false,
        ignore_message: ::core::option::Option::None,
        source_file: "gix-macros/tests/test.rs",
        start_line: 199usize,
        start_col: 4usize,
        end_line: 199usize,
        end_col: 22usize,
        compile_fail: false,
        no_run: false,
        should_panic: test::ShouldPanic::No,
        test_type: test::TestType::IntegrationTest,
    },
    testfn: test::StaticTestFn(|| test::assert_test_result(test_struct_method())),
};
fn test_struct_method() {
    match (
        &TestStruct
            .test_method("12345", "12345", String::from("12345"), S(true))
            .unwrap(),
        &"123451234512345!2345",
    ) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
    TestStruct
        .test_method("12345", "12345", String::from("12345"), S(false))
        .unwrap_err();
    match (
        &TestStruct
            .test_method2("12345", "12345", String::from("12345"), S(true))
            .unwrap(),
        &"123451234512345!2345",
    ) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
    TestStruct
        .test_method2("12345", "12345", String::from("12345"), S(false))
        .unwrap_err();
}
extern crate test;
#[cfg(test)]
#[rustc_test_marker = "test_struct_fn"]
pub const test_struct_fn: test::TestDescAndFn = test::TestDescAndFn {
    desc: test::TestDesc {
        name: test::StaticTestName("test_struct_fn"),
        ignore: false,
        ignore_message: ::core::option::Option::None,
        source_file: "gix-macros/tests/test.rs",
        start_line: 226usize,
        start_col: 4usize,
        end_line: 226usize,
        end_col: 18usize,
        compile_fail: false,
        no_run: false,
        should_panic: test::ShouldPanic::No,
        test_type: test::TestType::IntegrationTest,
    },
    testfn: test::StaticTestFn(|| test::assert_test_result(test_struct_fn())),
};
fn test_struct_fn() {
    match (
        &TestStruct::test_fn("12345", "12345", String::from("12345"), S(true), ())
            .unwrap(),
        &"123451234512345!2345",
    ) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
    TestStruct::test_fn("12345", "12345", String::from("12345"), S(false), ())
        .unwrap_err();
    match (
        &TestStruct::test_fn2(
                TestStruct,
                "12345",
                "12345",
                String::from("12345"),
                S(true),
                (),
                (),
            )
            .unwrap(),
        &"123451234512345!2345",
    ) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
    TestStruct::test_fn2(
            TestStruct,
            "12345",
            "12345",
            String::from("12345"),
            S(false),
            (),
            (),
        )
        .unwrap_err();
    match (
        &TestStruct::test_fn3(
                Pin::new(&mut TestStruct),
                "12345",
                "12345",
                String::from("12345"),
                S(true),
                (),
                (),
            )
            .unwrap(),
        &"123451234512345!2345",
    ) {
        (left_val, right_val) => {
            if !(*left_val == *right_val) {
                let kind = ::core::panicking::AssertKind::Eq;
                ::core::panicking::assert_failed(
                    kind,
                    &*left_val,
                    &*right_val,
                    ::core::option::Option::None,
                );
            }
        }
    };
    TestStruct::test_fn3(
            Pin::new(&mut TestStruct),
            "12345",
            "12345",
            String::from("12345"),
            S(false),
            (),
            (),
        )
        .unwrap_err();
}
#[rustc_main]
#[no_coverage]
pub fn main() -> () {
    extern crate test;
    test::test_main_static(&[&test_basic_fn, &test_struct_fn, &test_struct_method])
}

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this :)! I think this crate will be very useful to produce landing pads and be a foray into proc-macro magic for gitoxide. With it it could be possible to perform static validation of values as well, for instance.

With that in mind, would you mind planning other kinds of macros as well and call it gix-macros instead? (I you have a better name, please go for it though :))

As for how to proceed with this PR, my preference is to wait until the crate can be used in a gix function or method somewhere so that we can see its real-world benefits.

gix-momo/Cargo.toml Outdated Show resolved Hide resolved
@NobodyXu NobodyXu changed the title feature: Add new crate gix-momo feature: Add new crate gix-macros Aug 20, 2023
@NobodyXu NobodyXu marked this pull request as ready for review August 20, 2023 13:16
@NobodyXu
Copy link
Contributor Author

I will try to use it gix, but I think this PR is already ready for review and needs reviews since I'm not so familiar with proc-macros.

@Byron
Copy link
Member

Byron commented Aug 21, 2023

I think I should get to this within the next couple of days. I myself have neary-zero experience with it either 😅.

@NobodyXu NobodyXu marked this pull request as draft August 21, 2023 12:57
@NobodyXu NobodyXu force-pushed the feat/gix-momo branch 5 times, most recently from 4a13e0c to 47b2ac6 Compare August 22, 2023 13:36
@Byron
Copy link
Member

Byron commented Aug 22, 2023

If you rebase now the cargo deny failure should go away.

The memory corruption issue happens a lot these days and I don't know where that's coming from. Since I haven't been fiddling with unsafe lately, maybe it's due to a zlib update. As far as I can tell, this is only an issue with the max build, and never with the pure build which also performs these clones. I may hope that it disappears as sudden as it appeared.

@Byron
Copy link
Member

Byron commented Aug 23, 2023

I was wondering if this is the best upfront-investment of time given that monomorphism isn't a problem fi it's instantiated only once anyway. So I ran cargo bloat -n 200 and noticed quite a few duplications in plumbing crates. And for plumbing crates, I wouldn't want to have gix-macros as dependency anyway if it can be avoided by replacing the generics with dyn by hand. Some of the functions I saw would make that possible.

In other words, I'd wish for this endeavour to be driven by data so the highest-value lowest effort changes can be made first. Maybe you have other means of determining where a problem actually is though.

With that said, please do feel free to continue on this track if it's fun and rewarding, as another approach would be to try momo things and compare the sizes of the build-artifacts and compile times.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 23, 2023

I was wondering if this is the best upfront-investment of time given that monomorphism isn't a problem fi it's instantiated only once anyway.

Yes, it will.

The cargo-binstall CI timings (generated by passing --timings to cargo) shows that on fresh build, simple-git takes ~45s on x86_64 MacOS, a lot longer than it should given that it's quite simple and gix itself already takes ~45s to compile all the items.

The culprit here is the monomorphisation and it makes simple crates like that take incredibly long to compile.

Since simple-git is in workspace, it's not cached so every CI has to take the time to build it.

For local dev, that's more time waiting every time their crates using gix recompiles, even if it's trivial.

And for plumbing crates, I wouldn't want to have gix-macros as dependency anyway if it can be avoided by replacing the generics with dyn by hand. Some of the functions I saw would make that possible.

Well, Into and TryInto for example, cannot be passed as dyn unless you Box them, but that will do more harm than good.

AsRef and AsMut can be passed as dyn but there's not much point doing that given that their implementation is most likely trivially inlinable.

That's where gix_macros::momo comes handy, avoid the duplication by slapping a proc-macro over it, while retaining the convenience and readability of the original implementation.

Admittedly though momo still has some problems with TryInto that I will fix.

In other words, I'd wish for this endeavour to be driven by data so the highest-value lowest effort changes can be made first. Maybe you have other means of determining where a problem actually is though.

Adding features is definitely a low-hanging food compared to proc-macro, but it would require some extensive understanding of how gix internally uses dependencies.

Also I've already started this, so I'd like to finish this before doing other work.

With that said, please do feel free to continue on this track if it's fun and rewarding, as another approach would be to try momo things and compare the sizes of the build-artifacts and compile times.

I will slap it on a few functions in gix tonight and slowly add them to more functions.

Will post before and after compile time on this

@Byron
Copy link
Member

Byron commented Aug 23, 2023

And for plumbing crates, I wouldn't want to have gix-macros as dependency anyway if it can be avoided by replacing the generics with dyn by hand. Some of the functions I saw would make that possible.

Well, Into and TryInto for example, cannot be passed as dyn unless you Box them, but that will do more harm than good.

AsRef and AsMut can be passed as dyn but there's not much point doing that given that their implementation is most likely trivially inlinable.

When plumbing crates use impl AsRef or impl TryInto I usually do this just for my own convenience when writing tests, but I see how this is the wrong tradeoff. Also, I was not at all mindful about it. So to rephrase, these uses of generics in plumbing crates can be reviewed and probably replaced by the discrete type that is desired. Maybe that will have a positive effect as well.

There are rare cases where a trait is passed in, sometimes to Progress, sometimes to custom traits like in gix-pack, and only there it might be worth keeping them for maximum performance. At some point it's a trade-off and I would want to avoid putting dyn into all the places just so it's consistent.

Even though I don't like the idea of gix-macros in plumbing crates, I could imagine it not costing much as all plumbing crates already use thiserror, which pulls in the proc-macro dependency chain.

That's where gix_macros::momo comes handy, avoid the duplication by slapping a proc-macro over it, while retaining the convenience and readability of the original implementation.

Admittedly though momo still has some problems with TryInto that I will fix.

In other words, I'd wish for this endeavour to be driven by data so the highest-value lowest effort changes can be made first. Maybe you have other means of determining where a problem actually is though.

Adding features is definitely a low-hanging food compared to proc-macro, but it would require some extensive understanding of how gix internally uses dependencies.

Also I've already started this, so I'd like to finish this before doing other work.

That makes sense, and maybe setting feature toggles is more my line of work given that I have created this component-graph to facilitate it.

With that said, please do feel free to continue on this track if it's fun and rewarding, as another approach would be to try momo things and compare the sizes of the build-artifacts and compile times.

I will slap it on a few functions in gix tonight and slowly add them to more functions.

Will post before and after compile time on this

I am looking forward to seeing those numbers - they will definitely help to make this more concrete. Right now to me all of it is theoretical.

Great work by the way, thanks for the initiative and engagement 🙏.

@NobodyXu NobodyXu force-pushed the feat/gix-momo branch 9 times, most recently from de035f4 to 1934159 Compare August 23, 2023 12:38
@NobodyXu NobodyXu marked this pull request as ready for review August 25, 2023 12:05
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 25, 2023

@Byron While there are still a few TODOs, I believe this PR is ready for review.

Edit:

I would like to evaluate on the current approach I'm using.
I will post a benchmark shortly.

@NobodyXu
Copy link
Contributor Author

I benchmarked the compilation performance by running cargo b --release --timings in the root of gitoxide.

Before

    Finished release [optimized] target(s) in 1m 12s
image image

After

    Finished release [optimized] target(s) in 1m 12s
image image

Findings

Using momo does increase compilation time of gix slightly but reduce time that uses it.

So far I have only momoed a few functions, and due to the limit of momo, I wasn't able to put it over all generics that is eligable for it.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Aug 29, 2023

@Byron I've slapped momo over everywhere I can apply, please review again if you have time.

I will open a separate PR to de-momo functions using TryInto since there are only a few of them and they might use .map_err, which is really hard to parse.

I also found several functions using impl FnMut(...) and wonder if it makes sense to use a &mut dyn FnMut(...) instead.

@NobodyXu NobodyXu requested a review from Byron August 29, 2023 13:28
@Byron
Copy link
Member

Byron commented Sep 2, 2023

@Byron I've slapped momo over everywhere I can apply, please review again if you have time.

Finally I am getting around taking a look. Sorry for the delay.

I also found several functions using impl FnMut(...) and wonder if it makes sense to use a &mut dyn FnMut(...) instead.

gix is (or should be) using callbacks sparingly, but when they are used I'd love them to be as easy to use as possible. In these few cases, I wouldn't mind de-momoing by hand. I think the same does apply for TryInto as well, even though it's probably more prominent and thus might be worth it. In general, I'd love for gix-macros to stay as small, simple and fast as possible.

I am taking a close look now and hope to see some improvements to either compile time or binary size as well. Let's see.

@Byron
Copy link
Member

Byron commented Sep 2, 2023

An unscientific benchmark showed that the filesizes are a little lower when momo is used, whereas compile times are about the same.

Without Momo

-rwxr-xr-x 1 byron staff 9894161 Sep 2 19:39 target/release/ein
-rwxr-xr-x 1 byron staff 18991777 Sep 2 19:39 target/release/gix

With Momo

-rwxr-xr-x 1 byron staff 9765057 Sep 2 19:37 target/release/ein
-rwxr-xr-x 1 byron staff 18898161 Sep 2 19:37 target/release/gix

Of course, the mileage in other binaries may vary greatly.

One big piece of code to de-momo would certainly be those that use Progress, and getting there will be some work. As will be cutting down inter-crate duplication due to monomorphization.

Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this awesome crate!

I did a minor refactor and added docs as well as I could. While doing that, I imagined how I would use this macro and thought that the last thing I want is to accidentally increase the compile time, for example, by putting it on a method, function or place where it has no effect.

Would it be hard to make it emit a compile_error if it didn't do anything? I thought ty_conversions might be the key, but I wasn't sure if that misses something so didn't dare to try implementing it. Further, if it is easy enough, could there also be a test for that? I think I have seen a crate that can test compile-time characteristics, but forgot its name.

Once this is clarified, I think it's ready for merge on the merit of having demonstrated a light reduction in binary sizes, a trend that will certainly continue with additional work. I think the next steps could be to make Progress dyn-safe and drive forward the gix-* crates deduplication or (even explicit) dyn-ification.

gix-macros/src/lib.rs Outdated Show resolved Hide resolved
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Sep 3, 2023

Would it be hard to make it emit a compile_error if it didn't do anything?

That should be possible.

I thought ty_conversions might be the key, but I wasn't sure if that misses something so didn't dare to try implementing it.

Yes, if you add an is_empty() check for it and then write something similar to gix-macros/src/momo.rs#88, then it should work as intended.

Further, if it is easy enough, could there also be a test for that? I think I have seen a crate that can test compile-time characteristics, but forgot its name.

I think it can be done using a doc test, by marking it compile_fail:

/// ```compile_fail
/// let x = 5;
/// x += 2; // shouldn't  compile!
/// ```

Also, discover that apparently `momo` isn't effective where it should be
*or* the implementation is more complex.
@Byron
Copy link
Member

Byron commented Sep 3, 2023

Thanks a lot for your help! I gave it a shot and found a good way to test error conditions of proc macros. While trying to implement the is_empty() check as suggested I noticed that there were a lot of failures where I wouldn't expect them. Either there is more to ty_conversions, or something else is going on.

Could you help?
Thank you 🙏

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Sep 3, 2023

ty_conversions is where the conversions Into, AsRef, AsMut is detected and put into, you are using it the right way.

I can have a look 12h later.

`ty_conversions` only cover named generics, it doesn't cover
`impl Trait`.

I also changed the span for the error to `call_site`, otherwise it would
show that the `compile_error!` comes from the comment, which is
impossible.

Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
@NobodyXu NobodyXu requested a review from Byron September 7, 2023 12:40
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks again!

I was wondering if momo should be even stricter and bail if it can't remove all generics of a function, but then contemplated that less generics is already a win as it reduces to set of possible instantiations.

On this branch, I am getting 24.0s for a debug build (gix + ein) and 48.7s for a release build.

❯ ls -l ./target/release/gix ./target/release/ein ./target/debug/gix ./target/debug/ein
-rwxr-xr-x  1 byron  staff  36282417 Sep  7 15:13 ./target/debug/ein
-rwxr-xr-x  1 byron  staff  68070609 Sep  7 15:13 ./target/debug/gix
-rwxr-xr-x  1 byron  staff   9765057 Sep  7 15:15 ./target/release/ein
-rwxr-xr-x  1 byron  staff  18898161 Sep  7 15:15 ./target/release/gix

On main, I am getting 23.8s for a debug build (gix + ein) and 44.9s for a release build.

❯ ls -l ./target/release/gix ./target/release/ein ./target/debug/gix ./target/debug/ein
-rwxr-xr-x  1 byron  staff  36358865 Sep  7 15:18 ./target/debug/ein
-rwxr-xr-x  1 byron  staff  66358817 Sep  7 15:18 ./target/debug/gix
-rwxr-xr-x  1 byron  staff   9734657 Sep  7 15:18 ./target/release/ein
-rwxr-xr-x  1 byron  staff  18750001 Sep  7 15:18 ./target/release/gix

I realize now that this branch isn't on top of main yet, and that apparently release builds already got faster.

Let me merge and see if it has any effect.

Now on main, I am getting 22.8s for a debug build (gix + ein) and 45.4s for a release build.

❯ ls -l ./target/release/gix ./target/release/ein ./target/debug/gix ./target/debug/ein
-rwxr-xr-x  1 byron  staff  36345649 Sep  7 15:22 ./target/debug/ein
-rwxr-xr-x  1 byron  staff  66302433 Sep  7 15:22 ./target/debug/gix
-rwxr-xr-x  1 byron  staff   9757153 Sep  7 15:23 ./target/release/ein
-rwxr-xr-x  1 byron  staff  18781889 Sep  7 15:23 ./target/release/gix

Debug-sizes seem to have gotten better, but release sizes got a little worse. Probably these aren't necessarily deterministic, and since it's all marginal I think simply isn't anything to see here. This makes me doubt the usefulness of momo at least for gix and ein, but they are possibly special as they only call each function once anyway, or use the same input types consistently.

I will be looking forward of anything improved for cargo-binstall already.

@Byron Byron merged commit a1ed6a1 into GitoxideLabs:main Sep 7, 2023
18 checks passed
@NobodyXu NobodyXu deleted the feat/gix-momo branch September 7, 2023 13:29
@NobodyXu
Copy link
Contributor Author

NobodyXu commented Sep 7, 2023

@Byron It seems that this PR indeed improves gitoxide CI a lot

Before Merge branch dynification takes 44m, after Merge branch 'feat/gix-momo' takes 33m

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Sep 7, 2023

It mostly comes from CI test.yml, compiling tests on CI is speedup by 2m, documenting speedup by 10s, and not sure what the other 8m speedup comes from.

@Byron
Copy link
Member

Byron commented Sep 8, 2023

Sometimes I have the same and CI fluctuates so that a 32min job takes 42min. I was already eyeing enterprise grade CI or adding an own runner even. In a few years once I get the next machine I think I will make it beefy enough be able to continuously provide some CI runner machines for MacOS, Linux and Windows, with each of them as strong as my current dev machine. Dreams… 😁.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Sep 8, 2023

Sometimes I have the same and CI fluctuates so that a 32min job takes 42min.

Oops, I thought that it has improved the CI compilation time 😅

I was already eyeing enterprise grade CI or adding an own runner even.

I think it would be great if ee can form some organisation and provide CI for any Rust crate who needs them and unify our funds for larger instances.

cargo-binstall suffers the same thing though its CI is faster (usually aroud 15-20m), other crates would likely also want larger instances.

What makes it reasonable to have a dedicated runner for rust crates is that rust compilation is not embarrassingly parallel, this is especially true on release where binstall sets codegen-units to 1 and uses fat-lto.

It would improve with parallel frontend parallelisation, but cargo's compilation model will impose a limit on that, especially with build.rs (with compiling external libs in it blocking all dependents) and proc-macro.

In a few years once I get the next machine I think I will make it beefy enough be able to continuously provide some CI runner machines for MacOS, Linux and Windows, with each of them as strong as my current dev machine. Dreams… 😁.

I am already considering sponsoring you, for new machine I think you could build one Intel i7-13900 or something better, AMD's good at single-core which doesn't work well with rust compilation and testing.

For MacOS, definitely go with Mac Mini with at most 12 core and 32G or Mac Studio with at most 24 cores and 192G.

I had to admit, for apple products, their GPU cores are a waste to me, unless we gave it to someone who can use them...

@Byron
Copy link
Member

Byron commented Sep 8, 2023

What makes it reasonable to have a dedicated runner for rust crates is that rust compilation is not embarrassingly parallel, this is especially true on release where binstall sets codegen-units to 1 and uses fat-lto.

This is the worst for performance, indeed! I had it too, but decided it's not worth the struggle, even locally. Of course, reproducible builds don't come from using more than one codegen units, but it's I will postpone this as long as possible.

I am already considering sponsoring you, for new machine I think you could build one Intel i7-13900 or something better, AMD's good at single-core which doesn't work well with rust compilation and testing.

Actually, I love the idea of trying to get custom runners for Rust projects to use. Somehow I feel the Rust Foundation could provide these, they have all the data-center providers and already get some machines from them. Even a single server could already make a huge difference, even when shared across many projects.

I think it's worth pointing out that long CI times aren't a problem most of the time - they are just a lower bound on how faster one can make releases or merge PRs. Now the gitoxide CI is at 45min worst case, typically around 30m, and it has been for a while. Maybe that's just how it is and everything else is luxury.

But yeah, I don't really want a machine to stand at home and consume power all the time, while being mostly idle. So having a Mac Studio with maxed-out CPU cores seems like a way to have max performance while working, and enough spare to have many VMs to host ones own CI needs as well. But no matter how, it remains a luxury and I treat it as such :D.

had to admit, for apple products, their GPU cores are a waste to me, unless we gave it to someone who can use them...

My hope is that in a couple of years, when I can seriously think about updating hardware, there will be enough models available locally that run on the GPU. Then it might even be part of a normal workflow to run these locally, even though beefy machines are still required. Right now though, at least for my line of work, big GPUs are indeed a huge waste.

@NobodyXu
Copy link
Contributor Author

NobodyXu commented Sep 8, 2023

Actually, I love the idea of trying to get custom runners for Rust projects to use.

We could rent a server (not a VM) in someone with cheap electricity and network for rust projects to use.

It would be more affordable than renting VMs if we can guarantee it's used frequently, and since we don't care about the latency at all (500ms-1s is totally acceptable) and also don't need much bandwidth, putting it somewhere with cheap electricity but relatively mediocre network is ok.

Somehow I feel the Rust Foundation could provide these, they have all the data-center providers and already get some machines from them.

That's a good idea but not sure if feasible, I read https://kobzol.github.io/rust/rustc/2023/07/30/optimizing-rust-ci-2023.html and it seems that rust-lang/rust CI takes a lot time and their servers might also be overloaded.

Maybe that's just how it is and everything else is luxury.

Yes, we also accept this in cargo-binstall.

But yeah, I don't really want a machine to stand at home and consume power all the time, while being mostly idle.

Yeah, electricity bill price has increased dramatically, even in Australia where supposedly half of electricity is provided by renewable energy, it's more than 0.41 aud per kWh now in Sydney.

So having a Mac Studio with maxed-out CPU cores seems like a way to have max performance while working, and enough spare to have many VMs to host ones own CI needs as well. But no matter how, it remains a luxury and I treat it as such :D.

My hope is that in a couple of years, when I can seriously think about updating hardware, there will be enough models available locally that run on the GPU. Then it might even be part of a normal workflow to run these locally, even though beefy machines are still required. Right now though, at least for my line of work, big GPUs are indeed a huge waste.

Yes, for now I just want a GPu good enough for running desktop GUI.

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

Successfully merging this pull request may close these issues.

2 participants