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

Benchmarks for different header sizes #824

Merged
merged 7 commits into from
Aug 11, 2022
Merged

Benchmarks for different header sizes #824

merged 7 commits into from
Aug 11, 2022

Conversation

lexnv
Copy link
Contributor

@lexnv lexnv commented Jul 13, 2022 •
edited by niklasad1
Loading

This PR adds benchmarks for:

  • HTTP round trips with custom headers
  • WS handshake with custom headers

Results

HTTP

Benchmark 0 KiB header 1 KiB header 5 KiB header 25 KiB header 100 KiB Header
sync/http_custom_headers_round_trip 57.518 us 51.544 us 64.647 us 64.291 us 102.75 us
async/http_custom_headers_round_trip 65.822 us 62.544 us 72.057 us 63.859 us 131.97 us

WS

Benchmark 0 KiB header 1 KiB header 2 KiB header 4 KiB header
sync/ws_custom_headers_handshake 86.346 us 88.853 us 89.603 us 96.346 us
async/ws_custom_headers_handshake 90.601 us 89.546 us 89.251 us 91.919 us

@lexnv
bench: Expose headers for http clients
97f9389 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
bench: Add bench for custom HTTP headers
0089605 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
bench: Add bench for WS handshakes
6085906 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv requested a review from a team as a code owner July 13, 2022 12:43
@lexnv lexnv self-assigned this Jul 13, 2022
@lexnv
Fix clippy
db35a04 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
TarikGul
TarikGul reviewed Jul 13, 2022
View reviewed changes
benches/bench.rs Outdated Show resolved Hide resolved
TarikGul
TarikGul reviewed Jul 13, 2022
View reviewed changes
benches/bench.rs Outdated Show resolved Hide resolved
TarikGul
TarikGul approved these changes Jul 13, 2022
View reviewed changes
Copy link
Member

@TarikGul TarikGul left a comment

Choose a reason for hiding this comment

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

Small nit, but LGTM

@lexnv
bench: Use KIB instead of 1 * KiB
56cb07c 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@jsdw
Copy link
Collaborator

jsdw commented Jul 15, 2022

I'm surprised by the HTTP performance not changing! Are you sure that the benchmarks are actually testinng a handshake/http round trip? I might be reading wrong but offhand it looks like they just build an HttpClient/WsClient, and the HttpClientBuilder::build() fn isn't async and so doesn't do much interesting, and the WsClientBuilder spawns a background task but I'm not sure whether the bit you're measuring does much?

@niklasad1
Copy link
Member

I'm surprised by the HTTP performance not changing! Are you sure that the benchmarks are actually testinng a handshake/http round trip? I might be reading wrong but offhand it looks like they just build an HttpClient/WsClient, and the HttpClientBuilder::build() fn isn't async and so doesn't do much interesting, and the WsClientBuilder spawns a background task but I'm not sure whether the bit you're measuring does much?

The HTTP benchmark is only measuring the actual roundtrip which looks legit to me.
Not sure if the changed caching of headers helps here which is created in HttpClient::new()

Lemme try benchmarking much bigger headers

@jsdw
Copy link
Collaborator

jsdw commented Aug 3, 2022

I'd be interested in that! And ah yes, the http benchmarking code does more than I initially saw :)

niklasad1
niklasad1 reviewed Aug 8, 2022
View reviewed changes
benches/bench.rs
/// Bench WS handshake with different header sizes.
fn ws_custom_headers_handshake(rt: &TokioRuntime, crit: &mut Criterion, url: &str, name: &str, request: RequestType) {
let mut group = crit.benchmark_group(request.group_name(name));
for header_size in [0, KIB, 2 * KIB, 4 * KIB] {
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, I find it weird that soketto rejects 8 KIB when it has this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, I think that was the reason why I had to limit the benchmarks to 8 KiB unfortunately.

niklasad1
niklasad1 reviewed Aug 8, 2022
View reviewed changes
benches/bench.rs
name: &str,
request: RequestType,
) {
let method_name = request.methods()[0];
Copy link
Member

Choose a reason for hiding this comment

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

@lexnv

I changed that we just test this against one specific JSON-RPC call i.e, we don't really care about which call it is.
The only we care about is the overhead of the headers ^^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense to me since we only care about the header benchmarking, it should also reduce the bench time 😄

@niklasad1 niklasad1 merged commit d00ec85 into master Aug 11, 2022
@niklasad1 niklasad1 deleted the bench_custom_header branch August 11, 2022 11:23
niklasad1 added a commit that referenced this pull request Oct 4, 2022
@lexnv @niklasad1
Benchmarks for different header sizes (#824)
ecea3f4 
* bench: Expose headers for http clients

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* bench: Add bench for custom HTTP headers

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* bench: Add bench for WS handshakes

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Fix clippy

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* bench: Use KIB instead of `1 * KiB`

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* tweak http headers size

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
niklasad1 added a commit that referenced this pull request Oct 4, 2022
@lexnv @niklasad1
Benchmarks for different header sizes (#824)
2d73a58 
* bench: Expose headers for http clients

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* bench: Add bench for custom HTTP headers

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* bench: Add bench for WS handshakes

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* Fix clippy

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* bench: Use KIB instead of `1 * KiB`

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

* tweak http headers size

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Niklas Adolfsson <niklasadolfsson1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@niklasad1 niklasad1 niklasad1 approved these changes

@TarikGul TarikGul TarikGul approved these changes

Assignees

@lexnv lexnv

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lexnv @jsdw @niklasad1 @TarikGul