-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Rewrite StorePath class in C++ #3702
Conversation
On nix-env -qa -f '<nixpkgs>', this reduces maximum RSS by 20970 KiB and runtime by 0.8%. This is mostly because we're not parsing the hash part as a hash anymore (just validating that it consists of base-32 characters). Also, replace storePathToHash() by StorePath::hashPart().
I'm sad to see going back to c++ and removing rust from the build tooling altogether. To me, it seemed like a good foot-hold to build on top of, even if it is small and currently wasteful. I suppose next steps on future Rust work would require a more extensive project plan? |
The problem with a Rust migration is that it would take years (if it's ever completed), and in the meantime only people who know Rust and C++ can contribute to Nix. So without a viable migration plan, it's better to stick to C++ for now. |
I think with a dedicated sprint we could have done it in, say, 3 months. But I do agree it's better to stick to one language if we are not so sprinting----let's not pay the polyglot tax until will commit to making it temporary. |
Re @grahamc the plan I would like to see is:
I find rewrites much higher risk than refactors, as the type system helps far less, which is why I like to "pre-refactor" to keep the rewrite as simplistic as possible. I am not sure much more planning is needed than that. I would put greater emphasis on staffing than project planning, so step 2 gets done quickly. |
StorePath clone() const | ||
{ | ||
return StorePath(*this); | ||
} |
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.
I hope we can get rid of this and go back to just using idiomatic copy constructors, and STL copying in particular?
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.
Yes, but I didn't want to clutter this PR to much. Will do that afterwards.
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.
Sorry, I meant to say "after" and didn't. Great plan, happy to do some of that for you if you like :).
Maybe the minimal daemon could be the start of changing to rust? |
@kloenk :) Well, |
Im currently working on a project I call |
This PR rewrites the Rust implementation of
StorePath
in C++.StorePath
was an experiment in Rust/C++ interop. However, it's currently the only bit of Rust in Nix, and depending on a Rust compiler for a fairly trivial class is rather wasteful. (Originally we also used Rust for tarfile processing, but that has been replaced by libarchive.)We also get a minor speed up and maximum RSS reduction from using a different internal representation of
StorePath
.I also replaced
storePathToHash()
by the more efficientStorePath::hashPart()
.