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

client: Use async connection in async TPU client #25969

Merged
merged 6 commits into from
Jun 28, 2022

Conversation

joncinque
Copy link
Contributor

Problem

As part of #25383, the async TPU client needs to use underlying async connections when sending messages.

Summary of Changes

This builds on #25826 and #25878, adding the last part to use a nonblocking connection from a nonblocking TPU client. At the same time, it makes use of nonblocking connections by sending to all destinations at the same time in separate tasks.

Only the last commit matters for this PR.

Copy link
Contributor

@ryleung-solana ryleung-solana left a comment

Choose a reason for hiding this comment

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

Overall, looks fine. My only potential issue is that I'm not a super huge fan of yet another enum{UDP, Quic} type struct (that we had just gotten rid of with enum-dispatch), and matching with Quic and UDP with the functions of that enum (thankfully there are only 2 of them). Not a showstopper if there's no other way, but I'd still prefer if this wasn't there.

client/src/connection_cache.rs Outdated Show resolved Hide resolved
client/src/nonblocking/quic_client.rs Show resolved Hide resolved
client/src/connection_cache.rs Outdated Show resolved Hide resolved
@joncinque
Copy link
Contributor Author

I completely agree on the annoyance of the enum. We need some type that can either create a QUIC connection or a UDP connection -- is there something more basic that can do that? I couldn't come up with anything, which is why the enum was necessary.

@ryleung-solana
Copy link
Contributor

I completely agree on the annoyance of the enum. We need some type that can either create a QUIC connection or a UDP connection -- is there something more basic that can do that? I couldn't come up with anything, which is why the enum was necessary.

I think it may be possible to use enum-dispatch here just like we did with the different implementations of TPUConnection. For example, we might simply have a 2 implementations of the trait BaseTpuConnection whose implementations of the functions new_nonblocking_connection and new_connection return the UDP or Quic connections as appropriate

@ryleung-solana
Copy link
Contributor

Generally looks good to me, other than the nitpick mentioned above about possibly using enum-dispatch for BaseTpuConnection (having BaseTpuConnection be a trait with functions new_nonblocking_connection and new_connection for which we have Quic and UDP implementations)

@joncinque
Copy link
Contributor Author

I think it may be possible to use enum-dispatch here just like we did with the different implementations of TPUConnection. For example, we might simply have a 2 implementations of the trait BaseTpuConnection whose implementations of the functions new_nonblocking_connection and new_connection return the UDP or Quic connections as appropriate

I'm not sure if that's the right approach. With the TpuConnection case, it made sense because you needed a trait object that doesn't incur the cost of dynamic dispatch when calling send_*.

In this case, a new trait object unnecessarily increases complexity and may even make things worse -- we would be adding dynamic dispatching through enum_dispatch, where currently there isn't any. I'd have to read it into it further, but I imagine that enum_dispatch is generating the same code that we have here, where we dispatch based on an enum, and not a vtable.

@ryleung-solana
Copy link
Contributor

I think it may be possible to use enum-dispatch here just like we did with the different implementations of TPUConnection. For example, we might simply have a 2 implementations of the trait BaseTpuConnection whose implementations of the functions new_nonblocking_connection and new_connection return the UDP or Quic connections as appropriate

I'm not sure if that's the right approach. With the TpuConnection case, it made sense because you needed a trait object that doesn't incur the cost of dynamic dispatch when calling send_*.

In this case, a new trait object unnecessarily increases complexity and may even make things worse -- we would be adding dynamic dispatching through enum_dispatch, where currently there isn't any. I'd have to read it into it further, but I imagine that enum_dispatch is generating the same code that we have here, where we dispatch based on an enum, and not a vtable.

The enum-dispatch macro doesn't involve the use of any vtables - it basically just does what the current manually-written code does, namely:

    match self {
        BaseTpuConnection::Udp(udp_socket) => {
            UdpTpuConnection::new_from_addr(udp_socket.try_clone().unwrap(), addr).into()
        }
        BaseTpuConnection::Quic(quic_client) => {
            QuicTpuConnection::new_with_client(quic_client.clone(), stats).into()
        }
    }

matching an enum based on the 2 variations and calling the appropriate function on that variation. In this case, we're calling differently-named functions on the Quic and UDP variations, but IMHO, it's possible to refactor the BaseTpuConnection code into a trait, and have Quic and UDP implementations, which can then be dispatched using enum-dispatch (which does not involve any vtables), and if more functions are added to BaseTpuConnection, I think it would be worth it to do this. However, since we currently only have 2 functions, agreed that enum-dispatch would add more complexity than it would save. So seems fine to me for now.

@joncinque
Copy link
Contributor Author

Ok great, I misunderstood your comment -- I thought you were suggesting enum_dispatch for performance, when this essentially implements the num_dispatch bits by hand. Thanks for the confirmation that this is in fact all working with enums and not vtables.

@joncinque joncinque merged commit 2436a2b into solana-labs:master Jun 28, 2022
@joncinque joncinque deleted the full-async branch June 28, 2022 15:01
@pgarg66 pgarg66 added the v1.10 label Jul 8, 2022
mergify bot pushed a commit that referenced this pull request Jul 8, 2022
* client: Add nonblocking QuicTpuConnection implementation

* Remove integer arithmetic

* client: Support sync and async connections in cache

* client: Use async connection in async TPU client

* Address feedback

* Rename Connection -> BaseTpuConnection

(cherry picked from commit 2436a2b)

# Conflicts:
#	Cargo.lock
#	client/Cargo.toml
#	client/src/nonblocking/tpu_client.rs
pgarg66 added a commit that referenced this pull request Jul 8, 2022
…26505)

* client: Use async connection in async TPU client (#25969)

* client: Add nonblocking QuicTpuConnection implementation

* Remove integer arithmetic

* client: Support sync and async connections in cache

* client: Use async connection in async TPU client

* Address feedback

* Rename Connection -> BaseTpuConnection

(cherry picked from commit 2436a2b)

# Conflicts:
#	Cargo.lock
#	client/Cargo.toml
#	client/src/nonblocking/tpu_client.rs

* conflicts

* fix cargo lock

Co-authored-by: Jon Cinque <jon.cinque@gmail.com>
Co-authored-by: Pankaj Garg <pankaj@solana.com>
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.

None yet

3 participants