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

[ESI Runtime] Replace Cap'nProto with gRPC #7217

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Conversation

teqdruid
Copy link
Contributor

@teqdruid teqdruid commented Jun 20, 2024

gRPC is a lot easier to use owing to Cap'nProto's C++ implementation using likj.

Breaks MMIO, but we're not currently using it.

After spending a truely obnoxious amount of time fighting capnp and
libkj, we made the decision to switch to another RPC system. We're no
longer modeling and serializing message types in Capnp and we don't need
the performance which capnp/libkj RPC promises, so there's really no
need for the additional complexity. A slower system which is thread safe
should work fine.

This commit breaks the build in a pretty horrible way and is not
intended to be merged on its own. It simply breaks up the diff.
@teqdruid teqdruid added the ESI label Jun 20, 2024
@teqdruid teqdruid marked this pull request as ready for review June 21, 2024 05:01
@teqdruid teqdruid requested a review from dtzSiFive as a code owner June 21, 2024 05:01
@teqdruid teqdruid requested a review from mortbopet June 21, 2024 05:01
@teqdruid
Copy link
Contributor Author

teqdruid commented Jun 21, 2024

image

yep. that's what I expected. and that's with a bunch more code documentation.

Copy link
Contributor

@mortbopet mortbopet left a comment

Choose a reason for hiding this comment

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

Great to see that the diff in the non-cosim-backend code was relatively minor.

Based on the implementation and testing you've done on this, how is your initial gut feeling for gRPC vs capnproto? I'm thinking both in terms of compile-time and runtime-debugging that you went through.

lib/Dialect/ESI/runtime/CMakeLists.txt Show resolved Hide resolved
lib/Dialect/ESI/runtime/CMakeLists.txt Outdated Show resolved Hide resolved
lib/Dialect/ESI/runtime/cosim/cosim.proto Outdated Show resolved Hide resolved
lib/Dialect/ESI/runtime/cpp/lib/backends/Cosim.cpp Outdated Show resolved Hide resolved
@teqdruid
Copy link
Contributor Author

Great to see that the diff in the non-cosim-backend code was relatively minor.

Actually, it was non-existent!

Based on the implementation and testing you've done on this, how is your initial gut feeling for gRPC vs capnproto? I'm thinking both in terms of compile-time and runtime-debugging that you went through.

It was definitely easier to debug both compilation and runtime bugs; BUT, only because I'm more familiar with standard threading systems than libkj's async model. The 'googleability' aspect surprisingly didn't help much (the runtime errors I ran into didn't come up with any relevant results), though the larger number of examples did help.

Capnp RPC is more expressive, which I miss. It supports non-unary arguments/results and returning a capability (i.e. API), both of which help model a moderately complex system in a cleaner way. Also, capnp proto doesn't have NEARLY as many complex dependencies which probably would have made a FetchContent approach feasible. I would probably prefer Capnp if not for the C++ RPC implementation using libkj.

Actually, we would probably still be using capnp if libkj documentation was much better (notably the tour document) years ago when I first started using it and libkj had evolved to use newer C++ APIs instead of sticking with its own stuff (which pre-dated the newer C++ APIs). I just got fed up over the years dealing with libkj and didn't take the time to properly learn it based on the code documentation when I had time at the outset.

@teqdruid teqdruid merged commit 88eeb26 into main Jun 21, 2024
4 checks passed
@teqdruid teqdruid deleted the teqdruid/grpc-cosim branch June 21, 2024 21:22
@teqdruid
Copy link
Contributor Author

Note that the commit message on this is wrong. It does not break the build in a horrible way. I just forgot to remove that part from the message when merging. 🤦

mingzheTerapines pushed a commit to Terapines/circt that referenced this pull request Jun 27, 2024
After spending a truly obnoxious amount of time fighting capnp and
libkj, we made the decision to switch to another RPC system. We're no
longer modeling and serializing message types in Capnp and we don't need
the performance which capnp/libkj RPC promises, so there's really no
need for the additional complexity. A slower system which is thread safe
should work fine.

This commit breaks the build in a pretty horrible way and is not
intended to be merged on its own. It simply breaks up the diff.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants