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

Rewrite StorePath class in C++ #3702

Merged
merged 2 commits into from Jun 16, 2020
Merged

Rewrite StorePath class in C++ #3702

merged 2 commits into from Jun 16, 2020

Conversation

edolstra
Copy link
Member

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 efficient StorePath::hashPart().

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().
@grahamc
Copy link
Member

grahamc commented Jun 16, 2020

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?

@edolstra
Copy link
Member Author

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.

@Ericson2314
Copy link
Member

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.

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 16, 2020

Re @grahamc the plan I would like to see is:

  1. Refactor C++ to make more Rust-like with C++17 ideoms (e.g. std::optional, std::variant)
  2. Quickly and dumbly rewrite line-by-line, initial rewrite should have no interesting changes, but std::variant can become Rust enum
  3. Continue cleaning up once everything is in Rust.

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);
}
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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 :).

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 16, 2020

Maybe we can keep a rust branch in this repo without this, and with #3671 and #3450, to remind/inspire people to someday do such a thing.

@Kloenk
Copy link
Member

Kloenk commented Jun 16, 2020

Maybe the minimal daemon could be the start of changing to rust?

@Ericson2314
Copy link
Member

Ericson2314 commented Jun 16, 2020

@Kloenk :) Well, libnixstore is still the biggest library in Nix I think. I imagine working bottom-up with libnixutils and top-down with libexpr could maybe work, switching tacks whenever one gets stuck.

@maisiliym
Copy link

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?

Im currently working on a project I call uniks (it has also been named myx previously) which is a subset of a bigger project called sajban, in rust, and uniks is essentially a superman version of nix. I should have a draft release within weeks. I mostly hang out in #sajban-dev:matrix.org on matrix. I wont be binding to nix with C++, as you have already discovered for yourselves it is not a very good approach. Im currently heavily relying on flakes, adding my own type data in each, along with toJSON and fromJSON ... just wish derivation had a stdin argument to pass data to the builder through stdin.

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.

None yet

5 participants