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

use dynamic dispatch for visitors #41

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

srijs
Copy link
Contributor

@srijs srijs commented Apr 26, 2023

Following up from the discussion in #40, this changes the visitor pattern to use dynamic dispatch. In my case, this reduces the text section size attributed to prost-reflect by ~6.2% from 311.8KiB to 292.4KiB.

In parallel, I'm also working to create a simple benchmark for testing the impact of changing FieldDescriptorLike, which should result in a slightly larger amount of size reduction if things go well.

@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (27ca4f1) 75.34% compared to head (f061a1f) 75.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #41      +/-   ##
==========================================
+ Coverage   75.34%   75.38%   +0.03%     
==========================================
  Files          31       31              
  Lines        5253     5253              
==========================================
+ Hits         3958     3960       +2     
+ Misses       1295     1293       -2     
Impacted Files Coverage Δ
prost-reflect/src/descriptor/build/visit.rs 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andrewhickman
Copy link
Owner

Thanks! In case you hadn't spotted it, there's a small benchmark for decoding at https://github.com/andrewhickman/prost-reflect/blob/27ca4f10618cd69e0f408b4ae7db808ea1374f49/prost-reflect-tests/benches/decode.rs

@andrewhickman andrewhickman merged commit 64ca255 into andrewhickman:main Apr 26, 2023
@srijs srijs deleted the dyn-visitor branch April 26, 2023 23:11
@andrewhickman
Copy link
Owner

Incidentally, the google protobuf well-known type descriptors are currently included as a 113KB binary blob. I don't know if that would appear in the text section, but that's probably a decent chunk of the binary size.

It would be possible to put that behind a feature flag, although without it we can't properly handle uninterpreted options.

@srijs
Copy link
Contributor Author

srijs commented Apr 26, 2023

Ah neat, hadn't seen the benchmark! Looks like a great starting point to do some experimentation with FieldDescriptorLike, thanks.

Incidentally, the google protobuf well-known type descriptors are currently included as a 113KB binary blob.

Oh yes, that's a good point. Feature flagging it wouldn't help me because I need the WKT definitions, although I wonder why the protoset is so large for what feels like a handful of relatively simple types?

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

2 participants