PVF: Refactor argument passing

[mrcnski]

Host data

Right now if you want to add some data that is passed from the host startup to a worker, there is a long chain of calls it has to go through. It should be refactored somehow. For example, maybe we could pass around some super-objects to make adding new fields a one-line change. It would make such changes much easier in the future.

For an example, in this commit we added just one new parameter, cache_path, and look how noisy the commit is.

[x] Worker data

Done in #2486.

On the worker-side, we often pass around the worker kind, pid, and dir together. Perhaps there could be one WorkerInfo struct.

Other?

In general I felt like the PVF code is in dire need of cleaning up. Any other ideas would be highly appreciated.

[eagr]

Just skimming through the pvf codebase. Just some random ideas, not sure if they are applicable. :))

Structural

Further flatten the structure. Some should be nested, but nested is rarely better in my experience. For instance,

src/
  prepare/
    mod.rs
    pool.rs
    queue.rs
    worker_intf.rs

could this be flattened to

src/
  prepare_pool.rs
  prepare_queue.rs
  prepare_spawn.rs

This is actually more revealing to me, esp. when the pool and queue are both used in host on their own.

Also, the whole thing feels like it could be

common/
host/
execute-worker/
prepare-worker

which seems be easier to grok.

Dev UX

Rather than having inlined test modules, maybe consider moving them out, such that modifying the test code won't cause Cargo to recompile the lib crates. For smaller projects, this probably doesn't matter. But you know how crazy the complation time is. If this makes sense to you, I'm willing to test it out.

[mrcnski]

Thanks for the ideas @eagr! Overall I am not sure about them, but maybe I've just grown really accustomed to the way things are right now. I don't think we should change things up without a clear benefit, as it can be quite disruptive, but let me know if I am missing the benefits that you see. After a discussion we can put it to a vote with the other contributors. :)

1. About flattening and adding a prefix, it really seems to be strictly worse. 😅 Structure is helpful so why remove it? Is it difficult navigating the hierarchy? IMO it helps keep things separate when we have prepare-specific and execute-specific code.
2. About having a separate host directory, you mean having this kind of structure, and pvf itself contains no code?

pvf/
  host/
  execute-worker/
  [etc...]

I'm not sure. I think from an architectural perspective, having the host at the top-level makes more sense, because it's the entrypoint to the whole PVF system. That's why the structure is how it is, buuut I could see it making a lot of sense to rename pvf to pvf-host. Otherwise I'd keep the structure how it is, the workers are dependencies of the host and can continue living further down the filesystem hierarchy.
3. You mean the unit tests, right? If it improves the Dev UX we can for sure consider it. However, we should have some evidence that it helps, before making a disruptive change. Maybe you could do a quick experiment with one test module and get some numbers on compilation speed before and after. That said, it's a standard practice in Rust to have unit tests close to the code, so IMO the benefit would have to be pretty substantial for us to do it.

[eagr]

You mentioned that you're not fond of the way the code is organized. So I was thinking to brought the ideas up to see if you like them too :) If we are going to refactor, we might as well just do it good.

Structures are always subjective. I have a dislike for (unnecessary) nested structures, so I would try to avoid them when possible. An extra layer should exist for a good reason. Usually we use it to hide or group related modules.

// src/execute/mod.rs
mod queue;
mod worker_intf;

pub use queue::{start, PendingExecutionRequest, ToQueue};

But whate are we hiding or grouping here? If it makes sense, we could probably achieve the same thing with just one file in stead of three and one extra layer.

/src
  /execute
    mod.rs
    queue.rs
    worker_intf.rs

/src
  execute_queue.rs

See my point here? 😄 Probably not the best example, the rust-analyzer fellows do something similar. Notice those hir-* and ide-* crates. With a flat structure (when it's reasonable to have), people could see the whole picture at a glance.

[eagr]

Having a flat, or consistent structure (when appropriate) is not just something that's good to have. Sometimes, if prefix over nesting results in a consistent structure, ti gives us something to leverage. For example, in the Cargo.toml at the workspace root,

"bridges/primitives/chain-asset-hub-kusama",
"bridges/primitives/chain-asset-hub-polkadot",
"bridges/primitives/chain-asset-hub-rococo",
"bridges/primitives/chain-asset-hub-westend",
"bridges/primitives/chain-asset-hub-wococo",
"bridges/primitives/chain-bridge-hub-cumulus",
"bridges/primitives/chain-bridge-hub-kusama",
"bridges/primitives/chain-bridge-hub-polkadot",
"bridges/primitives/chain-bridge-hub-rococo",
"bridges/primitives/chain-bridge-hub-westend",
"bridges/primitives/chain-bridge-hub-wococo",
"bridges/primitives/chain-kusama",
"bridges/primitives/chain-polkadot",
"bridges/primitives/chain-polkadot-bulletin",
"bridges/primitives/chain-rococo",
"bridges/primitives/chain-westend",
"bridges/primitives/chain-wococo",

This could be turned into just one line

"bridges/primitives/*"

And you don't need to touch the manifest when adding new crates. I could also imagine the CI scripts would benefit from this somehow. If the team had established the convention of preferring flat structure with prefix at the beginning (I know it's not practical given the long history of the project), we would have a more consistent architecture and wouldn't have to maintain this giant member list.

My point is, a flat, consistent structure does have its value. If we're going to refactor the code base, those prepare and execute dirs could be easily turned into 3 files. Arguably, it also makes it clear what is involved.

src/
  ...
  prepare_pool.rs
  prepare_queue.rs
  execute_queue.rs
  ...

2 is a similar story. Exactly as you said :))

polkadot/node/core
  pvf-common/
  pvf-host/
  pvf-prepare-worker/
  pvf-execute-worker/

This eliminates the only exception in core/. Changes for 1 is trivial, but this is more significant. Is it doable?

I'll test 3 out to see if it makes sense. I'd say both ways of organizing tests are conventional. Inlined test modules make more sense for smaller projects where compilation is instant, so there is no reason to separate them.

[mrcnski]

Thanks for explaining @eagr, I get your point now and where you're coming from. But the lack of structure is definitely subjective and not my cup of tea. 😅 And I don't think there is a problem with our nesting that needs to be fixed. For (1), I'd say it's subjective, so not worth changing the existing convention. For (2), I think that where there is a hierarchy of components, like here, it doesn't make sense to obscure the hierarchy/structure by flattening the file layout into one level. But I'll definitely keep your points in mind for the future.

One case where we have some pretty deep nesting which we could consider changing is pvf/common/worker/security. I was aware the nesting could be viewed as excessive as I was doing that, but there was a logic to it. Everything in common is shared by at least two components, and common/worker is all code shared between the two workers. I'm open to moving it to worker-common, but I wanted to keep the number of crates we had low. Personally, I think the structure we have makes sense, it's not an area of the system that needs improving in my mind. There are also historical reasons for this layout, see paritytech/polkadot#7253 and #649.

[mrcnski]

There are also historical reasons for this layout, see paritytech/polkadot#7253 and #649.

To clarify, these are still relevant, not just historical. :P And of course I also forgot to mention paritytech/polkadot#7337.

[eagr]

Those are mere suggestions. If you don't think they are worth doing, let's just move on.

FWIW, when used right, the prefix way is great in that it somehow preserves the hierachy logic, at the same time gives you a more consistent structure, at the small price of duplication in dir names. For clarification, I totally agree that hierachy is necessary. Having over 400 crates lying at the same level is horrifying. 😆 I guess it's a matter of finding the sweet spot.

[eagr]

Inlined test module (on changing test code):

Separate test module (on changing test code):

~10s difference, at a small scale like this (only pvf modules), probably not worth changing.

[mrcnski]

Oh cool, thanks for testing it! I guess it's late and my brain isn't working, what am I supposed to be comparing? I don't see a 10s difference between any of the numbers there. If it is 10s, that sounds like a lot.

[eagr]

Sorry about that. It's 16s to be specific. But I have a crappy machine, so I rounded that down. :))

Here's what I have done.

1. build the tests in PVF at the current state
2. comment out a test case in host.rs
3. build againt with cargo test --no-run --timings, which gives figure 1
4. move the host tests out and build
5. repeat 2 & 3, which gives figure 2

Basically, with an inlined test module, the lib code is recompiled even if you only change the test code.

[mrcnski]

Basically, with an inlined test module, the lib code is recompiled even if you only change the test code.

Hrm I didn't know that! How old is your machine? I wouldn't make the effort to optimize for ancient computers, but if we can make a 10+s improvement for most people, that seems worth doing. And thanks for teaching me about the --timings option!

[eagr]

If you try to think about it, it's hard for Cargo to detect what exactly is changed coz they're in the same file. I mean for sure they could, just probably not worth doing as text matching and code isolation is equally expensive if not more. And it'd probably complicate compilation a lot.

I have a 2017 macbook, definitely not ancient. But compared to M2, it's kinda crappy, which probably takes ~3x as long to compile the project. I actually have to disable cargo check on save for rust-analyzer. The machine couldn't handle it.

[mrcnski]

Yeah, cargo only works on the file level, but I thought rustc would be able to detect if test-only code has changed... Anyway, I would say it's worth extracting the tests if you're up for it. I run tests on a VPS (for access to Linux) and it's painfully slow.

[eagr]

Just LMK if it's ok to separate the tests now? Don't wanna get in the way of your PRs. :)) I was hesitant about this coz I wasn't sure if ~10s is worth the trouble, esp. considering the compile time as a whole. Now that I think of it, it would probably make a difference for people doing cargo watch -x test.

[mrcnski]

Just LMK if it's ok to separate the tests now? Don't wanna get in the way of your PRs. :))

Thanks for asking. Yes, I don't think it would conflict with open work. :)

I was hesitant about this coz I wasn't sure if ~10s is worth the trouble, esp. considering the compile time as a whole.

Small savings here and there can add up over time. And I just realized that other crates tend to put unit tests in their own tests folder. Just keep in mind that there just some that don't make sense to move. For example, pre_encoded_payloads should stay close to OOM_PAYLOAD IMO.