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.

Sign up for GitHub

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

Jump to bottom

Add benchmarks #59

Merged
merged 62 commits into from
May 31, 2022
Merged

Add benchmarks #59

merged 62 commits into from
May 31, 2022

Conversation

CastilloDel
Copy link
Contributor

Closes #46

Before this is ready to get merge I need to add benchmarks for the htsget-search crate and I want to add running the benchmarks to the GitHub actions

brainstorm
brainstorm requested changes Aug 12, 2021
View reviewed changes
Copy link
Member

@brainstorm brainstorm left a comment

Choose a reason for hiding this comment

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

Fantastic work here, Daniel, well done!

Other than the couple of minor comments I left, I'd save one static benchmark that cleary shows the performance difference of the reference implementation against htsget-rs, as mentioned in Slack:

Where does criterion-rs store the different results for the run(s)? Does it commit the .jsons back to the repository or save it in the GitHub Actions CI’s cache?
I’m thinking about having some sort of static plot in the README and then refer to the series of plots generated across releases, so that people can easily track the progress?

htsget-http-actix/htsget-refserver-config.json Outdated Show resolved Hide resolved
htsget-search/benches/benchmark.rs Outdated Show resolved Hide resolved
@brainstorm
Copy link
Member

As for the commiting .htmls and jsons back to the repo... maybe https://github.com/boa-dev/criterion-compare-action is a better solution actually?

@CastilloDel
Copy link
Contributor Author

It seems the action doesn't work :(
Something about an unsupported option

error: Unrecognized option: 'save-baseline'
error: bench failed
Error: The process 'cargo' failed with exit code 101
    at ExecState._setResult (/home/runner/work/_actions/jasonwilliams/criterion-compare-action/move_to_actions/dist/index.js:1:7880)
    at ExecState.CheckComplete (/home/runner/work/_actions/jasonwilliams/criterion-compare-action/move_to_actions/dist/index.js:1:7442)
    at ChildProcess.<anonymous> (/home/runner/work/_actions/jasonwilliams/criterion-compare-action/move_to_actions/dist/index.js:1:6406)
    at ChildProcess.emit (events.js:210:5)
    at maybeClose (internal/child_process.js:1021:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:283:5)

I'll put an issue

@CastilloDel CastilloDel force-pushed the benchmarks branch 2 times, most recently from 86a1a99 to 0644688 Compare August 12, 2021 18:03
@CastilloDel
Copy link
Contributor Author

I think I found a work-around! Although it means the criterion action will have to run twice

@CastilloDel
Copy link
Contributor Author

Well, it failed again, but I think it is because it is trying to execute the benchmarks in the main branch, where it fails. Tomorrow I'll make a fork and try it 😥

brainstorm and others added 25 commits September 27, 2021 13:44
@brainstorm
Add missing actix_files::Files
998026e
@CastilloDel
Fix several errors related to the static file server
2af6bde
@CastilloDel
Fix benchmark so it can run as blocking
838f78e
@mmalenic
Merge branch 'main' of https://github.com/umccr/htsget-rs into benchm…
86be4dc 
…arks

� Conflicts:
�	.github/workflows/action.yml
�	htsget-http-actix/Cargo.toml
�	htsget-http-actix/src/handlers/blocking/get.rs
�	htsget-http-actix/src/handlers/blocking/mod.rs
�	htsget-http-actix/src/handlers/blocking/post.rs
�	htsget-http-actix/src/lib.rs
�	htsget-http-actix/src/main.rs
�	htsget-http-core/src/blocking/mod.rs
�	htsget-http-core/src/json_response.rs
�	htsget-http-core/src/lib.rs
�	htsget-search/Cargo.toml
�	htsget-search/src/htsget/bam_search.rs
�	htsget-search/src/htsget/bcf_search.rs
�	htsget-search/src/htsget/blocking/bam_search.rs
�	htsget-search/src/htsget/blocking/bcf_search.rs
�	htsget-search/src/htsget/blocking/cram_search.rs
�	htsget-search/src/htsget/blocking/from_storage.rs
�	htsget-search/src/htsget/blocking/vcf_search.rs
�	htsget-search/src/htsget/cram_search.rs
�	htsget-search/src/htsget/from_storage.rs
�	htsget-search/src/htsget/vcf_search.rs
�	htsget-search/src/storage/blocking/local.rs
�	htsget-search/src/storage/local.rs
@mmalenic
Merge branch 'main' of https://github.com/umccr/htsget-rs into benchm…
9b40653 
…arks
@mmalenic
Refactor benchmarks, fix a few errors, rearrange some tests.
50d78de
@brainstorm
Merge branch 'main' into benchmarks
5eca71a
@mmalenic
Refactor rcgen in axum server.
e7217e0
@mmalenic
Merge branch 'main' of https://github.com/umccr/htsget-rs into benchm…
65dbfd6 
…arks

� Conflicts:
�	.gitignore
@mmalenic
Merge branch 'benchmarks' of https://github.com/umccr/htsget-rs into …
8f89303 
…benchmarks

� Conflicts:
�	.gitignore
@mmalenic
Implement script files into rust code.
85a2d95
@mmalenic
Merge branch 'benchmarks' of https://github.com/umccr/htsget-rs into …
aaea724 
…benchmarks

� Conflicts:
�	htsget-http-actix/Cargo.toml
�	htsget-http-actix/benches/request-benchmark.rs
�	htsget-search/src/storage/axum_server.rs
@mmalenic
Merge branch 'main' of https://github.com/umccr/htsget-rs into benchm…
4fdf781 
…arks
@mmalenic
Fix localstorage path (#86)
c89fac3 
* Add more descriptive errors.

* LocalStorage no longer exposes path.

* Fix all tests related to new localstorage changes.

* Update README.
@mmalenic
Merge branch 'benchmarks' of https://github.com/umccr/htsget-rs into …
f44c52a 
…benchmarks

� Conflicts:
�	htsget-test-utils/src/server_tests.rs
@mmalenic
Fix certificate errors by using rustls-tls.
b252096
@mmalenic
Http byte ranges are inclusive for ending range, fix this and all aff…
25e364d 
…ected tests.
@mmalenic
Add light and heavy benchmarks, update README.md.
020a9ad
@mmalenic
Update actions, remove files, update README.md.
a528f65
@mmalenic
Update action
97d1875
@mmalenic
Resolve default/s3-storage feature clashes.
2a77de4
@mmalenic
Update action.yml.
da79a0f
@mmalenic
Update action.yml.
28e9347
@mmalenic
Update action
253f97f
@mmalenic
Reduce number of samples.
b13c410
@mmalenic
Copy link
Member

@brainstorm I think this might just be done now.

Thanks a lot @CastilloDel for all the code and guidance on how to structure the benchmarks.

Some recent changes:

  • Refactor search and request benchmarks to use recent changes in htsget-rs.
  • Some bug fixes in htsget-rs, such as byte ranges being inclusive for http headers.
  • Add benchmark actions to main action.yml - runs only light benchmarks.

brainstorm
brainstorm approved these changes May 31, 2022
View reviewed changes
Copy link
Member

@brainstorm brainstorm left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Amazing job, thanks @mmalenic! Nice forensics on the 1-off byte range bit, might have tricked you for a while :-!

Feel free to merge and move on to issues #85 and #75, you got this! :)

@mmalenic mmalenic merged commit eb8cf51 into main May 31, 2022
@brainstorm brainstorm deleted the benchmarks branch May 31, 2022 03:09
@github-actions github-actions bot mentioned this pull request May 4, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brainstorm brainstorm brainstorm approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Benchmarking
3 participants
@CastilloDel @brainstorm @mmalenic