-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
Conversation
e326fb5
to
c3f8c32
Compare
There was a problem hiding this 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.
c3f8c32
to
850fa1b
Compare
gix-momo
gix-macros
I will try to use it |
I think I should get to this within the next couple of days. I myself have neary-zero experience with it either 😅. |
4a13e0c
to
47b2ac6
Compare
If you rebase now the 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 |
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 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 |
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.
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.
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.
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 |
When plumbing crates use There are rare cases where a trait is passed in, sometimes to Even though I don't like the idea of
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.
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 🙏. |
de035f4
to
1934159
Compare
@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. |
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>
@Byron I've slapped I will open a separate PR to de- I also found several functions using |
Finally I am getting around taking a look. Sorry for the delay.
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. |
An unscientific benchmark showed that the filesizes are a little lower when Without Momo-rwxr-xr-x 1 byron staff 9894161 Sep 2 19:39 target/release/ein With Momo-rwxr-xr-x 1 byron staff 9765057 Sep 2 19:37 target/release/ein Of course, the mileage in other binaries may vary greatly. One big piece of code to de-momo would certainly be those that use |
There was a problem hiding this 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.
Signed-off-by: Jiahao XU <Jiahao_XU@outlook.com>
That should be possible.
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.
I think it can be done using a doc test, by marking it compile_fail:
|
Also, discover that apparently `momo` isn't effective where it should be *or* the implementation is more complex.
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 Could you help? |
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>
There was a problem hiding this 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 It seems that this PR indeed improves gitoxide CI a lot Before Merge branch |
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. |
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… 😁. |
Oops, I thought that it has improved the CI compilation time 😅
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.
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... |
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.
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 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.
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. |
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.
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.
Yes, we also accept this in cargo-binstall.
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.
Yes, for now I just want a GPu good enough for running desktop GUI. |
To make de-monomorphizatioin easier without sacrification of code readability as mentioned in #987
TODO
momo
: Put the inner function inside themomo
ed function for any non-self
function.E.g.
this: Self
and withoutSelf
.Support parsingimpl
block so that we can applymomo
to trait implementationmomo
: Remove unused generics inTryInto
: Removed anything related to itsError
momo
clone::PrepareFetch::new
momo
repository/object.rs
:Repository::commit_as
momo
discover.rsmomo
init.rs
momo
open::repository.rs
momo
create::into
mut
in the genericfn
except forAsMut
, and removemut
fromAsMut
target in innerfn
. User can opt-out of this by annotating#[keep]
in front of theAsMut
param name.Expanded test code
Command for getting expanded code:
cargo expand --test test
Actual expanded
tests/test.rs
code: