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

refactor splitting #1771

Merged
merged 1 commit into from
Oct 15, 2024
Merged

refactor splitting #1771

merged 1 commit into from
Oct 15, 2024

Conversation

rukai
Copy link
Member

@rukai rukai commented Oct 11, 2024

Attempt at DRY'ing up the splitting logic, seeing if its worth it or not.

fetch splitting deviates too far from the logic of the rest of the splitting methods, so I've not attempted to implement it with the new RequestSplitAndRouter trait.

Since this only saves 40 lines, normally I would say its not worth the complexity of traits here.
However there are two reasons I think we should go ahead with this:

  1. The logic that is DRY'd is actually pretty tricky to get right, hence why it has so many comments explaining whats going on. So having that implemented only once is quite valuable, on the other hand while the RequestSplitAndRouter trait implementations take ~25 lines each to implement, they are very difficult to get wrong since its mostly just specifying types so the type checker will catch anything wrong here.
  2. we are going to need to implement message splitting for at least 2 more message types and likely more. So this will save more lines in the future.

Copy link

codspeed-hq bot commented Oct 11, 2024

CodSpeed Performance Report

Merging #1771 will improve performances by 13.59%

Comparing rukai:refactor_splitting (678118c) with main (a3cef2c)

Summary

⚡ 1 improvements
✅ 38 untouched benchmarks

Benchmarks breakdown

Benchmark main rukai:refactor_splitting Change
encode_system.local_result_v5_no_compression 105.7 µs 93 µs +13.59%

@rukai rukai force-pushed the refactor_splitting branch from fad6f6a to baf3be6 Compare October 15, 2024 00:43
@rukai rukai force-pushed the refactor_splitting branch from baf3be6 to 678118c Compare October 15, 2024 02:56
@rukai rukai merged commit ce25cc4 into shotover:main Oct 15, 2024
41 checks passed
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.

3 participants