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

Add the wtx project #473

Merged
merged 2 commits into from
Sep 5, 2024
Merged

Add the wtx project #473

merged 2 commits into from
Sep 5, 2024

Conversation

c410-f3r
Copy link
Contributor

wtx is a RFC7541 and RFC9113 implementation written in Rust with built-in support for gRPC connections.

Here goes a local benchmark comparing java_vertex_grpc, dotnet_grpc and tonic_mt.

Capture d’écran du 2024-08-19 21-38-44

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Aug 22, 2024

Erhhh... That is strange... Didn't encounter these CI errors in my local run.

I will investigate

@c410-f3r
Copy link
Contributor Author

Should probably be fine now

Although the new results are not as interesting as before.

grpc

Copy link
Owner

@LesnyRumcajs LesnyRumcajs left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I left some comments.

rust_wtx_bench/Dockerfile Outdated Show resolved Hide resolved
rust_wtx_bench/Dockerfile Outdated Show resolved Hide resolved
rust_wtx_bench/Dockerfile Outdated Show resolved Hide resolved
rust_wtx_bench/src/main.rs Outdated Show resolved Hide resolved
@c410-f3r
Copy link
Contributor Author

Thanks for the review

@@ -0,0 +1,2 @@
[build]
rustflags = [ "-C", "target-cpu=native" ]
Copy link
Owner

Choose a reason for hiding this comment

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

Let's omit this flag. The reason it is not in other Rust implementations is that it:

  1. May result in an illegal instruction segfault in some CPUs.
  2. Image built on one machine may crash on another one.

There shouldn't be an assumption that the binary will be run on the exact same CPU where it was built. In practice, it's rarely the case.

Copy link
Contributor Author

@c410-f3r c410-f3r Aug 28, 2024

Choose a reason for hiding this comment

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

grpcio/tonic explicitly use this compiler flag and wtx performs manual vectorization when processing HPACK headers.

let mut iter = ArrayChunks::new(hpack_bytes);
for [a, b, c, d] in iter.by_ref() {
  // Process a...
  // Process b...
  // Process c...
  // Process d...
}

So target-cpu=native does have a great impact in the final generated assembly.

Trying a middle ground, I renamed everything from native to x86-64-v3. OK?

Copy link
Owner

Choose a reason for hiding this comment

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

To my understanding, the fact that libraries use a certain parameter doesn't matter much when using them in external projects. The flags that matter are defined in the target binary settings, e.g., here.

If we are to use this optimisation, it should be used across the board in all projects that have this option (all Rust ones, likely C/C++ as well). This can be potentially a follow-up PR.

Copy link
Contributor Author

@c410-f3r c410-f3r Sep 2, 2024

Choose a reason for hiding this comment

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

In trifectatechfoundation/zlib-rs#123, a benchmark that should have been tied resulted in a performance difference of 80% due to the lack of rustflags = [ "-C", "target-cpu=native" ] (https://www.phoronix.com/news/Gentoo-x86-64-v3-Binaries).

Rustc, as well as gcc/clang, defaults to a conservative set of x86 instructions and while the performance difference may not be as huge as the linked issue, some slowdown is likely going to occur.

$ rustc --print target-cpus

    x86-64                  - This is the default target CPU for the current build target (currently x86_64-unknown-linux-gnu).
    x86-64-v2
    x86-64-v3
    x86-64-v4

But yeah, I will just remove this parameter from all projects to make things more fair. Is that OK?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, it's my bad. I think I didn't check through the hidden directories it seems. Sorry for that!

Let's have it they way you had it originally, with the native in wtx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, no problem!

@c410-f3r c410-f3r force-pushed the master branch 2 times, most recently from 2354648 to 0e10c05 Compare September 2, 2024 10:05
WORKDIR /app
COPY rust_wtx_bench /app
COPY proto /app/protos
RUN apt-get update && apt-get install gcc -y
Copy link
Owner

Choose a reason for hiding this comment

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

It's likely not needed.

❯ docker run --rm rust:1.76 gcc --version
gcc (Debian 12.2.0-14) 12.2.0
Suggested change
RUN apt-get update && apt-get install gcc -y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

Copy link
Owner

Choose a reason for hiding this comment

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

Is this supposed to utilize multiple threads as well? If so, I recommend setting the thread workers count manually, same as in https://github.com/LesnyRumcajs/grpc_bench/blob/66229fa6aae1f832b8f4642ea894131372a6ffd0/rust_tonic_mt_bench/src/main.rs#L31C9-L40. Otherwise, the app will likely see host CPUs and distribute the workload suboptimally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goodness, thank you for spotting this oversight.

Copy link
Owner

@LesnyRumcajs LesnyRumcajs left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@LesnyRumcajs LesnyRumcajs merged commit 38875a7 into LesnyRumcajs:master Sep 5, 2024
47 checks passed
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 5, 2024

Thank you @LesnyRumcajs

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.

2 participants