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

test/scale crate out of date #1105

Open
rrybarczyk opened this issue Aug 15, 2024 · 6 comments
Open

test/scale crate out of date #1105

rrybarczyk opened this issue Aug 15, 2024 · 6 comments
Labels
clean-up test Test related

Comments

@rrybarczyk
Copy link
Collaborator

rrybarczyk commented Aug 15, 2024

Background

stratum/test contains three crates:

  1. config/: Contains each role's toml config for the message-generator test stack.
  2. message-generator: Generates messages to test SRI.
  3. scale: Outputs the time spent sending 1,000,000 SubmitSharesStandard throughout the system. It contains a main.rs binary, which means it will generate a Cargo.lock when built. Thi

When working on enforcing no changes to Cargo.lock in #1039 (also related to #1044 and #1102), each crate was investigated to see which contains a main.rs to enforce the no change in lock files during the pre-push call to scripts/clippy-fmt-and-test.sh.

Problem

It is unclear exactly how/when scale should be used. When running cargo build in test/scale, a versioning error on the network_helpers_sv2 is encountered. This indicates to me that this crate is very out of date and is not really used.

The only reference to the scale crate is within the test/scale directory itself. Perhaps this indicates accumulated dust and should just be removed.

Furthermore, in the scripts/clippy-fmt-and-test.sh, the test directory containing the scale crate is not included in these checks. If we keep this test/scale crate, should we include it in the scripts/clippy-fmt-and-test.sh?

cargo build
    Updating crates.io index
error: failed to select a version for the requirement `network_helpers_sv2 = "^0.1"`
candidate versions found which didn't match: 2.0.0
location searched: /Users/rachelrybarczyk/Development/StratumV2/Dev/stratum/roles/roles-utils/network-helpers
required by package `scale v1.0.0 (/Users/rachelrybarczyk/Development/StratumV2/Dev/stratum/test/scale)`

Solution

Understand what test/scale is used for. If it is needed, update it so it runs properly. If it is not needed, remove it and all references to it.

@rrybarczyk rrybarczyk added test Test related clean-up labels Aug 15, 2024
@rrybarczyk
Copy link
Collaborator Author

@Fi3 can you provide clarity on test/scale?

@Fi3
Copy link
Collaborator

Fi3 commented Aug 17, 2024

@darricksee ?

@rrybarczyk
Copy link
Collaborator Author

Just spoke with @darricksee and explained the following:

The test simply outputs the time spent sending 1,000,000 SubmitSharesStandard through the system. When you start the test you specify -h -e (for encryption). The test spawns "proxies" (ports 19000 -> 19000+) which simply decrypt/encrypt each SubmitSharesStandard message coming in (if encryption is on). Then it sends 1,000,000 share messages to the first proxy and then times the whole system to see how long it takes for the last proxy to receive all 1M messages. It uses the same network_helpers that the pool and proxies use, so it should be a good approximation of the work they do.

The supported run flags are:

  • -h <int>: Number of hops
  • -e: If present, turns encryption on. Otherwise no encryption is used.

For example, to run with 4 hops and encryption on:

cargo run --release -- -h 4 -e

NOTE: running without --release dramatically shows down the test.

This test/scale crate is intended to be ran before/after making a change that may impact performance. Ideally it would be run automatically and the build would fail if performance is drastically worse for a commit.

@Fi3, given this context, I think this could be something useful to update and keep around.

I like the idea of having some checks to make sure we are not making changes that hurt performance, before we get too far in solidifying those changes.

Thinking about performance monitoring also made me wonder if we should also be using something like flamegraph-rs to watch out for inefficient implementation logic.

Now that we have a MVP, and as we move onto the refactor and optimization phase, I think we need to start being more mindful of code performance.

@GitGab19, any thoughts?

@GitGab19
Copy link
Collaborator

Code performance is definitely something we need to care about.

Regarding the feature about having checks on every PR to ensure we do not introduce regressions, that's the reason behind run-benchmarks.yaml, track-benchmarks, and run-and-track-benchmarks-on-main.yaml. They use criterion and iai to execute benches defined here: https://github.com/stratum-mining/stratum/tree/dev/benches.

But it seems something is not working properly (as described by #1051); in addition to this, I think benches defined there are incomplete and not that much helpful at the end. @Fi3 I don't know if you agree with me on this.

To summarize, I think that some kind of tests like this one could me more helpful than what we have now, so I'm up for reconsidering the way we are checking our code performance 👍

@GitGab19
Copy link
Collaborator

Also, we currently don't know how our codebase scale at all, since we never tested it with more than 2 machines pointed to the translator. So I strongly believe we should put our focus here. I think that benchmarking-tool can also help with this, depending on how strongly it will be used

@plebhash plebhash added this to the 1.2.0 milestone Sep 9, 2024
@plebhash
Copy link
Collaborator

plebhash commented Sep 9, 2024

ok, I just put this issue on 1.2.0 milestone so we can address this in the future

we should eventually have a call to discuss the scope of the work that is needed here before we start getting our hands dirty

@rrybarczyk @GitGab19 @Shourya742

@plebhash plebhash removed this from the 1.2.0 milestone Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clean-up test Test related
Projects
Status: Todo 📝
Development

No branches or pull requests

4 participants