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

[ci] Additional variables for benchmarks execution #884

Merged
merged 5 commits into from
Sep 28, 2022
Merged

Conversation

alvicsam
Copy link
Contributor

@alvicsam alvicsam commented Sep 26, 2022

Added MEASUREMENT_TIME: 3 and SLOW_MEASUREMENT_TIME: 40 variables to benchmark job. With these variables it takes ~2h to run benchmarks: https://gitlab.parity.io/parity/mirrors/jsonrpsee/-/jobs/1885548

cc https://github.com/paritytech/ci_cd/issues/302

@alvicsam alvicsam requested review from a team as code owners September 26, 2022 15:23
@alvicsam alvicsam changed the title [WIP][ci] Fix benchmarks [ci] Additional variables for benchmarks execution Sep 27, 2022
@niklasad1
Copy link
Member

niklasad1 commented Sep 27, 2022

I suggest to make slow stuff 60s but I have done some investigation and with some improvement it still took ~3h to complete.

I need to refactor the benches to be faster but the bad the thing is that we can't compare with the old data then :(

See #885

I still think 3h is way too long.

@alvicsam
Copy link
Contributor Author

I need to refactor the benches to be faster but the bad the thing is that we can't compare with the old data then :(

If we consider some data not accurate maybe it's not that bad?

I still think 3h is way too long.

But why? Because for local run it's too long?

@niklasad1
Copy link
Member

If we consider some data not accurate maybe it's not that bad?

Probably yes, but I can try backporting the benches to the "most recent" releases to have some data compare against at least.

But why? Because for local run it's too long?

Yeah or unnecessary load on the benchmarks itself, we are testing with some extreme limits such a calls with 1 MB payload, slow call with a sleep for 1 ms and up to 1024 concurrent requests.

I think we can relax the payload size and sleep time to make it run "reasonable time (less than 1 hour)" without any impact. Trying it right now locally...

@alvicsam
Copy link
Contributor Author

alvicsam commented Sep 27, 2022

Also it's possible to run benchmarks with cargo-nextest after criterion support is added to nextest: nextest-rs/nextest#96. It can significantly reduce time

.gitlab-ci.yml Outdated Show resolved Hide resolved
@niklasad1
Copy link
Member

I just merged #885 but I think we should try with MEASUREMENT_TIME=10 to begin with it's just used a two benches now.

@alvicsam
Copy link
Contributor Author

Okay, I'd like to wait https://gitlab.parity.io/parity/mirrors/jsonrpsee/-/pipelines/217578 to check how long the benches will take with this settings. If the job succeeds I'll merge this PR

@alvicsam alvicsam merged commit 45c94a2 into master Sep 28, 2022
@alvicsam alvicsam deleted the as-benches branch September 28, 2022 08:04
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.

3 participants