Open rrybarczyk opened 1 month ago
@Fi3 can you provide clarity on test/scale
?
@darricksee ?
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?
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 👍
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
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
Background
stratum/test
contains three crates:config/
: Contains each role's toml config for themessage-generator
test stack.message-generator
: Generates messages to test SRI.scale
: Outputs the time spent sending 1,000,000SubmitSharesStandard
throughout the system. It contains amain.rs
binary, which means it will generate aCargo.lock
when built. ThiWhen working on enforcing no changes to
Cargo.lock
in #1039 (also related to #1044 and #1102), each crate was investigated to see which contains amain.rs
to enforce the no change in lock files during thepre-push
call toscripts/clippy-fmt-and-test.sh
.Problem
It is unclear exactly how/when
scale
should be used. When runningcargo build
intest/scale
, a versioning error on thenetwork_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 thetest/scale
directory itself. Perhaps this indicates accumulated dust and should just be removed.Furthermore, in the
scripts/clippy-fmt-and-test.sh
, thetest
directory containing thescale
crate is not included in these checks. If we keep thistest/scale
crate, should we include it in thescripts/clippy-fmt-and-test.sh
?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.