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

H-3422: harpc: rework the family of Codec traits #5362

Merged
merged 23 commits into from
Oct 10, 2024
Merged

Conversation

indietyp
Copy link
Member

@indietyp indietyp commented Oct 9, 2024

🌟 What is the purpose of this PR?

This reworks the Codec trait, to internally use serde, while this makes serde a hard dependency. I think that's overall fine, as it allows us to more forward more quickly, and in any case - all of Rust is built on serde anyway.

The Encoder and Decoder traits haven't been removed, instead, they take a stream of items and convert them to bytes.
I first tried to re-use tokio-util here, but found that to be quite cumbersome - especially in trait bounds, because our Codec ensures that only serde items are used we don't have any additional trait bound, that's not the case with tokio-util, we'd need to write everywhere: Encoder<AccountId> etc, which would essentially re-create the problem we already had.

This is quite a large PR, simply because the Codec trait was pretty heavily used. The trait was previously part of harpc-net but has been moved into harpc-codec. It just felt... inappropriate to have the definition of the encoder and decoder on the network layer if it is going to be used for values on the tower layer.

Errors still have their own encoder and decoder, simply because we encode two different things, errors and reports and deserialization of reports isn't possible just yet.

To proof the concept a new codec has been added: JsonCodec, which implements said traits and is now used throughout all tests.

next steps are moving some of the types in harpc-wire-protocol into harpc-codec. Reason for this is that it feels wrong that harpc-service and harpc-tower need to depend on harpc-wire-protocol, even tho it is a completely different layer.

This was one of the biggest hurdles that we faced, so I am glad this is out of the way now!

Pre-Merge Checklist 🚀

🚢 Has this modified a publishable library?

This PR:

  • does not modify any publishable blocks or libraries, or modifications do not need publishing

📜 Does this require a change to the docs?

The changes in this PR:

  • are internal and do not require a docs change

🕸️ Does this require a change to the Turbo Graph?

The changes in this PR:

  • affected the execution graph, and the turbo.json's have been updated to reflect this

@indietyp indietyp requested a review from a team as a code owner October 9, 2024 18:21
@indietyp indietyp requested a review from TimDiekmann October 9, 2024 18:21
@github-actions github-actions bot added area/deps Relates to third-party dependencies (area) area/infra Relates to version control, CI, CD or IaC (area) area/libs Relates to first-party libraries/crates/packages (area) type/eng > backend Owned by the @backend team type/legal Owned by the @legal team labels Oct 9, 2024
@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@indietyp
Copy link
Member Author

indietyp commented Oct 9, 2024

looking at the changes in the Cargo.toml it seems like the removal of workspace dependencies is from cargo add (or similar?) it removed all dependencies that were unused in the workspace.

@vilkinsons vilkinsons changed the title harpc: rework the family of Codec traits. H-3422: harpc: rework the family of Codec traits. Oct 10, 2024
@vilkinsons vilkinsons changed the title H-3422: harpc: rework the family of Codec traits. H-3422: harpc: rework the family of Codec traits Oct 10, 2024
Copy link
Member

@TimDiekmann TimDiekmann left a comment

Choose a reason for hiding this comment

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

I think it's the right call to just rely on serde. if we ever need/want to support other formats we can look into lift this and find a common interface, but to get things moving this is better I guess. Thank you!

I have a few comments, but mostly these are niceties/code style.

Comment on lines -202 to -203
temporal-io-client = { git = "https://github.com/temporalio/sdk-core", rev = "7e3c23f" }
temporal-io-sdk-core-protos = { git = "https://github.com/temporalio/sdk-core", rev = "7e3c23f" }
Copy link
Member

Choose a reason for hiding this comment

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

They are used but not as workspace dependency. Could you instead adjust @local/temporal-client to use workspace = true instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

see c8e5f8c

serde_plain = { version = "=1.0.2", default-features = false }
serde_with = { version = "=3.11.0", default-features = false }
similar-asserts = { version = "=1.6.0", default-features = false }
spin = { version = "=0.9", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

Spin is used in error-stack but with a >=0.9 bound. Probably good to remove it, yes.

publish.workspace = true

[dependencies]
error-stack = { workspace = true, public = true, features = ["serde"] }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
error-stack = { workspace = true, public = true, features = ["serde"] }
# Public workspace dependencies
error-stack = { workspace = true, public = true, features = ["serde"] }

Copy link
Member Author

Choose a reason for hiding this comment

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

see: 96d959c


pub trait ErrorDecoder {
type Error;
type Recovery;
Copy link
Member

Choose a reason for hiding this comment

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

What is Recovery? Do you have a quick example?

Copy link
Member Author

Choose a reason for hiding this comment

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

see: eabf6aa

Copy link
Member

Choose a reason for hiding this comment

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

I see, thanks!

libs/@local/harpc/codec/src/error.rs Outdated Show resolved Hide resolved
libs/@local/harpc/codec/src/error.rs Show resolved Hide resolved
libs/@local/harpc/codec/src/error.rs Outdated Show resolved Hide resolved
libs/@local/harpc/net/Cargo.toml Outdated Show resolved Hide resolved
libs/@local/harpc/net/src/session/error.rs Show resolved Hide resolved
libs/@local/harpc/server/Cargo.toml Outdated Show resolved Hide resolved
TimDiekmann
TimDiekmann previously approved these changes Oct 10, 2024
vilkinsons
vilkinsons previously approved these changes Oct 10, 2024
Copy link
Contributor

Benchmark results

@rust/graph-benches – Integrations

representative_read_entity

Function Value Mean Flame graphs
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/block/v/1 $$15.8 \mathrm{ms} \pm 168 \mathrm{μs}\left({\color{lightgreen}-36.104 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/song/v/1 $$15.8 \mathrm{ms} \pm 200 \mathrm{μs}\left({\color{lightgreen}-5.188 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/organization/v/1 $$15.8 \mathrm{ms} \pm 198 \mathrm{μs}\left({\color{lightgreen}-35.345 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/book/v/1 $$15.7 \mathrm{ms} \pm 207 \mathrm{μs}\left({\color{lightgreen}-39.448 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/person/v/1 $$15.7 \mathrm{ms} \pm 174 \mathrm{μs}\left({\color{gray}-3.451 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/uk-address/v/1 $$16.9 \mathrm{ms} \pm 253 \mathrm{μs}\left({\color{gray}-0.924 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/page/v/2 $$16.5 \mathrm{ms} \pm 177 \mathrm{μs}\left({\color{red}7.22 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/playlist/v/1 $$15.4 \mathrm{ms} \pm 165 \mathrm{μs}\left({\color{lightgreen}-5.779 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id entity type ID: https://blockprotocol.org/@alice/types/entity-type/building/v/1 $$16.2 \mathrm{ms} \pm 183 \mathrm{μs}\left({\color{gray}3.75 \mathrm{\%}}\right) $$ Flame Graph

representative_read_entity_type

Function Value Mean Flame graphs
get_entity_type_by_id Account ID: d4e16033-c281-4cde-aa35-9085bf2e7579 $$1.40 \mathrm{ms} \pm 6.62 \mathrm{μs}\left({\color{gray}-3.011 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_complete_zero_depth

Function Value Mean Flame graphs
entity_by_id 50 entities $$4.02 \mathrm{ms} \pm 26.8 \mathrm{μs}\left({\color{gray}-2.345 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10 entities $$2.04 \mathrm{ms} \pm 12.8 \mathrm{μs}\left({\color{gray}-0.557 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 25 entities $$2.72 \mathrm{ms} \pm 82.3 \mathrm{μs}\left({\color{red}9.19 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1 entities $$1.86 \mathrm{ms} \pm 10.9 \mathrm{μs}\left({\color{gray}-0.157 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 5 entities $$1.88 \mathrm{ms} \pm 11.7 \mathrm{μs}\left({\color{gray}-0.254 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_linkless

Function Value Mean Flame graphs
entity_by_id 1000 entities $$2.76 \mathrm{ms} \pm 24.0 \mathrm{μs}\left({\color{gray}1.35 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 100 entities $$2.03 \mathrm{ms} \pm 9.61 \mathrm{μs}\left({\color{gray}1.55 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10 entities $$1.87 \mathrm{ms} \pm 8.97 \mathrm{μs}\left({\color{gray}0.025 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10000 entities $$12.3 \mathrm{ms} \pm 125 \mathrm{μs}\left({\color{gray}-2.373 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1 entities $$1.86 \mathrm{ms} \pm 6.06 \mathrm{μs}\left({\color{gray}-0.127 \mathrm{\%}}\right) $$ Flame Graph

representative_read_multiple_entities

Function Value Mean Flame graphs
entity_by_property depths: DT=2, PT=2, ET=2, E=2 $$59.5 \mathrm{ms} \pm 295 \mathrm{μs}\left({\color{gray}2.52 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=255, PT=255, ET=255, E=255 $$68.8 \mathrm{ms} \pm 418 \mathrm{μs}\left({\color{gray}2.33 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=0, E=0 $$40.4 \mathrm{ms} \pm 248 \mathrm{μs}\left({\color{gray}4.31 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=0, E=2 $$43.9 \mathrm{ms} \pm 240 \mathrm{μs}\left({\color{gray}2.32 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=2, ET=2, E=2 $$55.1 \mathrm{ms} \pm 254 \mathrm{μs}\left({\color{gray}2.71 \mathrm{\%}}\right) $$ Flame Graph
entity_by_property depths: DT=0, PT=0, ET=2, E=2 $$50.8 \mathrm{ms} \pm 283 \mathrm{μs}\left({\color{gray}2.86 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=2, PT=2, ET=2, E=2 $$100 \mathrm{ms} \pm 692 \mathrm{μs}\left({\color{gray}1.84 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=255, PT=255, ET=255, E=255 $$109 \mathrm{ms} \pm 563 \mathrm{μs}\left({\color{gray}1.51 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=0, E=0 $$42.8 \mathrm{ms} \pm 327 \mathrm{μs}\left({\color{gray}-0.338 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=0, E=2 $$80.3 \mathrm{ms} \pm 414 \mathrm{μs}\left({\color{gray}-2.474 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=2, ET=2, E=2 $$95.1 \mathrm{ms} \pm 498 \mathrm{μs}\left({\color{gray}1.41 \mathrm{\%}}\right) $$ Flame Graph
link_by_source_by_property depths: DT=0, PT=0, ET=2, E=2 $$90.3 \mathrm{ms} \pm 441 \mathrm{μs}\left({\color{gray}-0.031 \mathrm{\%}}\right) $$ Flame Graph

scaling_read_entity_complete_one_depth

Function Value Mean Flame graphs
entity_by_id 50 entities $$272 \mathrm{ms} \pm 2.22 \mathrm{ms}\left({\color{gray}2.60 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 10 entities $$46.6 \mathrm{ms} \pm 2.44 \mathrm{ms}\left({\color{lightgreen}-5.328 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 25 entities $$71.7 \mathrm{ms} \pm 366 \mathrm{μs}\left({\color{gray}-0.526 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 1 entities $$19.7 \mathrm{ms} \pm 96.2 \mathrm{μs}\left({\color{gray}-0.874 \mathrm{\%}}\right) $$ Flame Graph
entity_by_id 5 entities $$25.2 \mathrm{ms} \pm 211 \mathrm{μs}\left({\color{gray}0.955 \mathrm{\%}}\right) $$ Flame Graph

@indietyp indietyp added this pull request to the merge queue Oct 10, 2024
Merged via the queue into main with commit 9fd7fef Oct 10, 2024
161 checks passed
@indietyp indietyp deleted the bm/harpc/cleanup branch October 10, 2024 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deps Relates to third-party dependencies (area) area/infra Relates to version control, CI, CD or IaC (area) area/libs Relates to first-party libraries/crates/packages (area) type/eng > backend Owned by the @backend team type/legal Owned by the @legal team
Development

Successfully merging this pull request may close these issues.

3 participants