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

Miscellaneous improvements to make the project more idiomatic #2

Merged
merged 7 commits into from
Aug 14, 2023

Conversation

Molter73
Copy link
Contributor

This PR makes a series of changes in order to use more idiomatic patterns in the project.

The first change is to make more use of serde (which is used by the config crate behind the curtains) in order to greatly simplify the code needed for deserializing the configuration. There's a somewhat controversial implication to these changes, since I also changed the format used in the configuration toml file. IMO the result is a more intuitive and simple configuration for the different workloads, but it is up for discussion (I added example workloads under the workloads/ directory).

Next major change was to use a Worker trait and split the worker.rs file into its own module. This way I was able to implement 3 separate workers (one for processes, one for endpoints and one for syscalls), so instead of having a massive file with everything under the same struct we now have a more organized set of objects with their run_payload() method. Using a helper new_worker function that returns a Worker and with the previous changes made to configuration deserialization, the code in main.rs becomes smaller and more generic.

Finally I made some small changes to the way information about the workers is displayed in order to reduce some of the boiler plaiting and added a step to our CI in order to run tests via cargo test

@Molter73 Molter73 requested a review from erthalion July 18, 2023 08:24
@erthalion
Copy link
Collaborator

Thanks for the PR, looks interesting. Trying to build it I get couple of errors like this:

error[E0658]: `let...else` statements are unstable
  --> src/worker/processes.rs:40:9
   |
40 | /         let Workload::Processes {
41 | |             arrival_rate: _,
42 | |             departure_rate: _,
43 | |             random_process,
44 | |         } = self.workload.workload else { unreachable!() };
   | |___________________________________________________________^
   |
   = note: see issue #87335 <https://github.com/rust-lang/rust/issues/87335> for more information

Any idea where is it coming from?

@Molter73
Copy link
Contributor Author

Molter73 commented Aug 9, 2023

Thanks for the PR, looks interesting. Trying to build it I get couple of errors like this:

error[E0658]: `let...else` statements are unstable
  --> src/worker/processes.rs:40:9
   |
40 | /         let Workload::Processes {
41 | |             arrival_rate: _,
42 | |             departure_rate: _,
43 | |             random_process,
44 | |         } = self.workload.workload else { unreachable!() };
   | |___________________________________________________________^
   |
   = note: see issue #87335 <https://github.com/rust-lang/rust/issues/87335> for more information

Any idea where is it coming from?

My guess is you have an older compiler, let else has been stable since 1.65, see: rust-lang/rust#93628

If you want to keep the older compiler, we could change the statements to look something like this

let arrival_rate = if let Workload::Syscalls { arrival_rate } = self.workload.workload { arrival_rate } else {unreachable!()};

But I think using the latest compiler is the way to go (most rust projects either keep up with the latest version or even go on to use the nightly builds).

@erthalion
Copy link
Collaborator

You're right, seems to be working after the update. Although now I'm concerned about the speed of changes under my feet, the version I was using is 1.62 :)

@Molter73
Copy link
Contributor Author

Molter73 commented Aug 9, 2023

You're right, seems to be working after the update. Although now I'm concerned about the speed of changes under my feet, the version I was using is 1.62 :)

Rust moves fairly fast, it's up to us to decide whether we want to use latest and greatest or just re-write for an older version. Because backwards compatibility is really good in Rust and they have extensive testing to proof newer compilers don't break older code, the community tends to always stay at latest, but I understand your concerns.

Copy link
Collaborator

@erthalion erthalion left a comment

Choose a reason for hiding this comment

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

Thanks, looks good. The workload interface and configuration are more clear now, and everything still seems to work. Few commentaries:

@@ -0,0 +1,64 @@
use std::fmt::Display;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this file called mod?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mod.rs is the default file for modules in rust, everything in this file will be found under worker (so you'd do things like worker::{BaseConfig, Worker}. All other files are considered submodules, so for using the EndpointWorker you do worker::endpoints::EndpointWorker, it's merely an organizational thing.

struct BaseConfig {
cpu: CoreId,
process: usize,
lower: usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

In fact lower/upper are leftovers of the mashed together config structure. It belongs to the endpoinds workload, and the only reason it had to be handled somewhat special is they're calculated in a stateful way, so that every single worker get a non overlapping interval.

Could you move it out from BaseConfig into Endpoints part, and introduce another interface function, something like "prepare_state(cpu, process)", that will either keep a mutable state or do something like a fold over list of cpu + processes to calculate lower/upper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up with a slightly different approach, having the new_worker function take the bounds mutably and changing them with each iteration. There's probably a better way to do this, but I'm clearly not smart enough.

The upper and lower bounds are only used by the endpoint worker, so it
is now removed from other types. The new_worker function now takes
mutable references to these bounds and the generation of independent
port ranges is delegated to this function. This also means the workers
must now be created by the parent process before forking itself in order
to have one source of truth for the ranges (but this should be fine,
since in the parent the worker goes out of scope right after forking).
@Molter73 Molter73 requested a review from erthalion August 11, 2023 11:28
Copy link
Collaborator

@erthalion erthalion left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@Molter73 Molter73 merged commit 571f8b2 into main Aug 14, 2023
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