[WIP] refactor(sdk): Switch "Client" impl from dynamic dispatch to static dispatch#1912
Closed
vax-r wants to merge 2 commits into
Closed
[WIP] refactor(sdk): Switch "Client" impl from dynamic dispatch to static dispatch#1912vax-r wants to merge 2 commits into
vax-r wants to merge 2 commits into
Conversation
… trait Add FromConnectionString trait as a public interface for structs which implement "from_connection_string()" It serves as a basis for future factor of IggyClient so methods that differs from different client type could be abstract to a common interface. Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
…ispatch Previously, the implementation of "Client" trait or "Client" object utilize "dyn Client" , which use dynamic dispatch at run-time. Refactor this implementation to generic type "T: Client" which allows the type to be dispatched statically. Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
vax-r
added a commit
to vax-r/iggy
that referenced
this pull request
Jul 3, 2025
Replace Box<dyn Client> trait objects with a concrete ClientWrapper enum to eliminate dynamic dispatch overhead and reduce heap allocations. By leveraging match-based delegation in ClientWrapper, we can eliminate dynamic dispatch and Box allocations for client instances. The enum is stack-allocated, compared to heap-allocated Box, saving heap memory. The ClientWrapper enum currently contains Http, Tcp, Quic, and Iggy variants. Future protocol implementations should be added as new variants. This change serves as a V2 implementation and alternative to the approach in apache#1912. As discussed, using pure static dispatch would alter too many use case behaviors and require users to have deeper API knowledge. With the enum wrapper, no API changes are needed, benefiting IggyClient, IggyConsumer, IggyProducer, and other components without changing how they are used. Performance improvements: - IggyClient::from_connection_string() execution time reduced by 3.45% - Average byte allocation reduced from 18,626 to 18,618 bytes - Allocation speed improved by 3.53% Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
vax-r
added a commit
to vax-r/iggy
that referenced
this pull request
Jul 3, 2025
Replace Box<dyn Client> trait objects with a concrete ClientWrapper enum to eliminate dynamic dispatch overhead and reduce heap allocations. By leveraging match-based delegation in ClientWrapper, we can eliminate dynamic dispatch and Box allocations for client instances. The enum is stack-allocated, compared to heap-allocated Box, saving heap memory. The ClientWrapper enum currently contains Http, Tcp, Quic, and Iggy variants. Future protocol implementations should be added as new variants. This change serves as a V2 implementation and alternative to the approach in apache#1912. As discussed, using pure static dispatch would alter too many use case behaviors and require users to have deeper API knowledge. With the enum wrapper, no API changes are needed, benefiting IggyClient, IggyConsumer, IggyProducer, and other components without changing how they are used. Performance improvements: - IggyClient::from_connection_string() execution time reduced by 3.45% - Average byte allocation reduced from 18,626 to 18,618 bytes - Allocation speed improved by 3.53% Signed-off-by: I Hsin Cheng <richard120310@gmail.com>
spetz
pushed a commit
that referenced
this pull request
Jul 4, 2025
## Summary Replace `Box<dyn Client>` trait objects with a concrete `ClientWrapper` enum to eliminate dynamic dispatch overhead and reduce heap allocations. By leveraging match-based delegation in `ClientWrapper`, we can eliminate dynamic dispatch and `Box` allocations for client instances. The enum is stack-allocated, compared to heap-allocated Box, saving heap memory. The `ClientWrapper` enum currently contains Http, Tcp, Quic, and Iggy variants. Future protocol implementations should be added as new variants. This change serves as a V2 implementation and alternative to the approach in #1912. As discussed, using pure static dispatch would alter too many use case behaviors and require users to have deeper API knowledge. With the enum wrapper, no API changes are needed, benefiting `IggyClient, IggyConsumer, IggyProducer`, and other components without changing how they are used.
Contributor
Author
|
Closed via #1958 |
hageshiame
pushed a commit
to hageshiame/iggy
that referenced
this pull request
Nov 7, 2025
## Summary Replace `Box<dyn Client>` trait objects with a concrete `ClientWrapper` enum to eliminate dynamic dispatch overhead and reduce heap allocations. By leveraging match-based delegation in `ClientWrapper`, we can eliminate dynamic dispatch and `Box` allocations for client instances. The enum is stack-allocated, compared to heap-allocated Box, saving heap memory. The `ClientWrapper` enum currently contains Http, Tcp, Quic, and Iggy variants. Future protocol implementations should be added as new variants. This change serves as a V2 implementation and alternative to the approach in apache#1912. As discussed, using pure static dispatch would alter too many use case behaviors and require users to have deeper API knowledge. With the enum wrapper, no API changes are needed, benefiting `IggyClient, IggyConsumer, IggyProducer`, and other components without changing how they are used.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR aims to refactor the usage of
dyn ClienttoT: Client.This is still a broken solution, the basis are almost done as in the commits. Currently the way to use it will be the main problem, as you can see in benchmarks and integration tests. When using
dyn Client, it grants us the convenience to implement single function to adapt numerous client type. Now switching toIggyClient<T: Client>,IggyConsumer<T: Client>, we need to put the type in explicitly, e.g.IggyClient::<HttpClient>, so it's hard to single function to be used to form a complete test.Main difficulties are listed below, maybe we should discuss before proceeding.
ClientFactory::create_client(), it's given an user-specific config and the program will figure out which protocol it is using. Now we must figure out the type of protocol first and callcreate_client::<protocol_type>so it can return a concrete type e.g.IggyClient<TcpClient>rather thanBox<dyn Client>.create_client()from my point of view, cuz SDK can't assume what protocol it will get from user.create_tcp_client()and so on doesn't help us get through the issue listed above, looks like when we can't assume the actual type ofT: Clientin advance, we needdyn Client?get_raw_client()and its related helpers suffers the same issue, we can get around this withunsafeRust. However, when using it in actual cases, we need to find a way to give concrete type forT, check the case incore/tools/src/data-seeder/main.rscreate_client(),get_raw_client(). Might even be separated as cases for http, quic, tcp and mixtures of them. Not sure if this is wanted.