cli: introduce host-perf-check command #4342

slumber · 2021-11-21T22:41:44Z

Resolves #4196

Creating as a draft since there're couple of unresolved questions so the code needs to be polished.
But the general approach is here so we have a starting point.

drahnr

A few nits, generally looks like you're on a good path.

cli/src/cli.rs

cli/src/command.rs

- Improve error handling - Keep subcommands available in both debug and release builds

cli/src/command.rs

cli/src/error.rs

cli/src/host_perf_check.rs

slumber · 2021-11-24T12:15:53Z

Does this support MacOS as well? With the M1s we will see a shift of more people building and maybe running them there as well.

We do 2 *nix things here: get $HOME env value and call gethostname from libc. As far as I know both are supposed to work the same way on MacOS and Linux. Testing it is as simple as:

cargo run --release host-perf-check

multiple times to verify it's cached.

Can only confirm it for Linux myself unfortunately.

slumber · 2021-11-24T13:37:33Z

@drahnr so we are fine with the rest? I will add erasure code test and optional run on the first launch then before marking it as ready-for-review.

drahnr · 2021-11-24T13:40:49Z

@drahnr so we are fine with the rest? I will add erasure code test and optional run on the first launch then before marking it as ready-for-review.

I am rather uneasy about all the deps pulled in, either we use something more abstract, i.e. dirs or directories for this and hostname for cross paltform hostname support or ditch it altogether. Implicitly assuming nobody is build/running this on MacOS will just bring problems.

slumber · 2021-11-24T13:57:05Z

I am rather uneasy about all the deps pulled in, either we use something more abstract, i.e. dirs or directories for this and hostname for cross paltform hostname support or ditch it altogether. Implicitly assuming nobody is build/running this on MacOS will just bring problems.

dirs for home directory I suppose? On macOS and Linux it's essentially reading $HOME, so I don't think it's even worth?
As for the nix as a new dependency, it's already here in the workspace, so I thought it's better to link an existing crate than compile a new one.

cli/src/host_perf_check.rs

bkchr · 2021-11-24T14:57:13Z

I am rather uneasy about all the deps pulled in, either we use something more abstract, i.e. dirs or directories for this and hostname for cross paltform hostname support or ditch it altogether. Implicitly assuming nobody is build/running this on MacOS will just bring problems.

dirs for home directory I suppose? On macOS and Linux it's essentially reading $HOME, so I don't think it's even worth? As for the nix as a new dependency, it's already here in the workspace, so I thought it's better to link an existing crate than compile a new one.

We have the base-path for this. We store there the dbs and other stuff.

cli/src/host_perf_check.rs

cli/src/command.rs

slumber · 2021-12-03T14:23:17Z

There're no constant multipliers yet, we will probably soften the limit with respect to reference values -- tbd.

drahnr

👍

ordian · 2021-12-03T14:45:21Z

cli/src/command.rs

+	{
+		Err(PerfCheckError::WrongBuildType.into())
+	}
+	#[cfg(build_type = "release")]


will this work with custom profiles #4311 (comment) @bkchr?

I don't think so, the profile should be matched with the one used for distributing the binary. If this profile changes to e.g release-lto, it should be also updated here.

bkchr

While we don't officially support windows, I think that we at least compiled on windows all the time.

I get why you want to store the host name there, but it still feels weird.

I mean users could already run into more trouble when the just blindly copy the entire base path, because they could be slashed for equivocations etc. Especially as the check is currently optional anyway? Aka needs to be executed by the user.

cli/src/command.rs

node/test/performance-test/build.rs

node/test/performance-test/src/lib.rs

node/test/performance-test/src/gen_ref_constants.rs

cli/src/host_perf_check.rs

cli/src/cli.rs

slumber · 2021-12-04T19:30:33Z

While we don't officially support windows, I think that we at least compiled on windows all the time.

I suppose it is already broken in node/core/pvf where we import unix related stuff (sockets in particular)

I get why you want to store the host name there, but it still feels weird.

I mean users could already run into more trouble when the just blindly copy the entire base path, because they could be slashed for equivocations etc. Especially as the check is currently optional anyway? Aka needs to be executed by the user.

This is a minor effort we can afford in order to protect users from it isn't it?

bkchr · 2021-12-05T14:29:59Z

I suppose it is already broken in node/core/pvf where we import unix related stuff (sockets in particular)

Okay, good point :)

Caching is for the case of running the check automatically on the first validator start. This is a consideration from the issue description.

Is that already done? No or?

slumber · 2021-12-05T17:18:37Z

Caching is for the case of running the check automatically on the first validator start. This is a consideration from the issue description.

Is that already done? No or?

Not yet, I'm unsure about the way of implementing it, it should always be optional so:

either it's always on and we allow skipping it with --no-perf-check (not cool for development environment)
or it's off by default and --with-perf-check enables it, but high chances people miss it or forget about it.

cc @drahnr

drahnr · 2021-12-05T18:01:58Z

Given that we don't want any potential issues with node restarts, on-by-default is not an option. Online host migrations pose also an issue, when the username changes, or the hostname is changed and on next restart it experiences the delay.

That's all not good UI, and I'd prefer to ship the partial feature, having just the explicit command and post pone the rest.

bkchr · 2021-12-05T18:52:35Z

Given that we don't want any potential issues with node restarts, on-by-default is not an option. Online host migrations pose also an issue, when the username changes, or the hostname is changed and on next restart it experiences the delay.

That's all not good UI, and I'd prefer to ship the partial feature, having just the explicit command and post pone the rest.

But then we don't need any of this caching feature? If this is something that stays optional and requires the user to run the command, why cache it?

drahnr · 2021-12-05T19:15:08Z

Fully agree, deferring the caching and the resolution seems to me like the best approach. @slumber could remove the file related caching values, the rest is good to go!

…ck-cli

drahnr · 2021-12-09T11:25:33Z

bot merge

paritytech-processbot · 2021-12-09T11:25:43Z

Error: Checks failed for 66dfa6d

cli: introduce host-perf-check command

d21db4e

slumber requested review from drahnr and pepyakin November 21, 2021 22:41

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 21, 2021

fix build

5937ce6

drahnr suggested changes Nov 22, 2021

View reviewed changes

slumber added 3 commits November 23, 2021 02:25

Review fixes

ff6d3a1

- Improve error handling - Keep subcommands available in both debug and release builds

Extract perf check into new module

71b19d3

Explicit log level

6770d63

bkchr reviewed Nov 23, 2021

View reviewed changes

cli/src/command.rs Outdated Show resolved Hide resolved

slumber added 3 commits November 23, 2021 15:38

Use a build script for cfg build

e142424

Save dummy file in HOME

9414d2f

Refactor

c6a72a8

drahnr reviewed Nov 24, 2021

View reviewed changes

cli/src/command.rs Outdated Show resolved Hide resolved

drahnr reviewed Nov 24, 2021

View reviewed changes

cli/src/error.rs Show resolved Hide resolved

drahnr reviewed Nov 24, 2021

View reviewed changes

cli/src/host_perf_check.rs Outdated Show resolved Hide resolved

slumber added 2 commits November 24, 2021 13:53

Error on any non-release build

a4f75a2

Improve naming

65701ad

bkchr reviewed Nov 24, 2021

View reviewed changes

cli/src/host_perf_check.rs Outdated Show resolved Hide resolved

cli/src/host_perf_check.rs Outdated Show resolved Hide resolved

slumber added 2 commits November 24, 2021 19:56

Use the base path for caching

30e225f

Reuse wasm binary from kusama_runtime

7bb37a2

slumber requested review from drahnr and bkchr November 24, 2021 22:06

drahnr reviewed Nov 25, 2021

View reviewed changes

cli/src/host_perf_check.rs Outdated Show resolved Hide resolved

drahnr reviewed Nov 25, 2021

View reviewed changes

cli/src/command.rs Outdated Show resolved Hide resolved

slumber requested a review from drahnr December 3, 2021 14:04

drahnr approved these changes Dec 3, 2021

View reviewed changes

ordian reviewed Dec 3, 2021

View reviewed changes

bkchr reviewed Dec 3, 2021

View reviewed changes

slumber added 4 commits December 5, 2021 00:42

Simplify fs read/write

28ecac3

Propagate tracing error

ab46e65

Expect instead of unwrap

04d311e

Update headers

f9ae107

Rename cache_path to base_path

f8739ae

slumber mentioned this pull request May 12, 2022

feature request: generate reference constants for the performance check paritytech/command-bot#59

Closed

Drop caching

9bdbddd

drahnr approved these changes Dec 5, 2021

View reviewed changes

Apply suggestions from code review

f36888d

bkchr approved these changes Dec 5, 2021

View reviewed changes

slumber added 2 commits December 9, 2021 12:19

Merge remote-tracking branch 'origin/master' into slumber-hostperfche…

e1e42e4

…ck-cli

Decrease the number of warm up runs

66dfa6d

drahnr merged commit e6e8da7 into master Dec 9, 2021

drahnr deleted the slumber-hostperfcheck-cli branch December 9, 2021 11:26

slumber mentioned this pull request Dec 9, 2021

cli: increase the time limit for the host-perf-check command #4498

Merged

librelois mentioned this pull request Feb 7, 2022

Update substrate/polkadot/cumulus from v0.9.15 to v0.9.16 moonbeam-foundation/moonbeam#1259

Closed

xlc mentioned this pull request Feb 8, 2022

host-perf-check sub command paritytech/substrate#10818

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: introduce host-perf-check command #4342

cli: introduce host-perf-check command #4342

slumber commented Nov 21, 2021

drahnr left a comment

slumber commented Nov 24, 2021

slumber commented Nov 24, 2021

drahnr commented Nov 24, 2021

slumber commented Nov 24, 2021

bkchr commented Nov 24, 2021

slumber commented Dec 3, 2021

drahnr left a comment

ordian Dec 3, 2021

slumber Dec 3, 2021

bkchr left a comment

slumber commented Dec 4, 2021

bkchr commented Dec 5, 2021

slumber commented Dec 5, 2021

drahnr commented Dec 5, 2021

bkchr commented Dec 5, 2021

drahnr commented Dec 5, 2021

drahnr commented Dec 9, 2021

paritytech-processbot bot commented Dec 9, 2021

cli: introduce host-perf-check command #4342

cli: introduce host-perf-check command #4342

Conversation

slumber commented Nov 21, 2021

drahnr left a comment

Choose a reason for hiding this comment

slumber commented Nov 24, 2021

slumber commented Nov 24, 2021

drahnr commented Nov 24, 2021

slumber commented Nov 24, 2021

bkchr commented Nov 24, 2021

slumber commented Dec 3, 2021

drahnr left a comment

Choose a reason for hiding this comment

ordian Dec 3, 2021

Choose a reason for hiding this comment

slumber Dec 3, 2021

Choose a reason for hiding this comment

bkchr left a comment

Choose a reason for hiding this comment

slumber commented Dec 4, 2021

bkchr commented Dec 5, 2021

slumber commented Dec 5, 2021

drahnr commented Dec 5, 2021

bkchr commented Dec 5, 2021

drahnr commented Dec 5, 2021

drahnr commented Dec 9, 2021

paritytech-processbot bot commented Dec 9, 2021