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

remove source code info from wkt bin #42

Closed

Conversation

srijs
Copy link
Contributor

@srijs srijs commented Apr 27, 2023

Based on your hint in #41, I went and looked at the well_known_types.bin file that's included in the binary, and noticed that the file descriptors all include source_code_info. In fact, the source_code_info on the files accounts for most of the size of well_known_types.bin.

This patch here adds a build script that removes the source code info from the file before including it in the binary. It results in a reduction in binary size of ~99KB, which based on the size of the WKT file means that we're reducing its size from ~115KB down to ~16KB.

We could consider just modifying the file directly in source control rather than using a build script, but I'm not sure how the file was sourced or how it will be updated in the future. The build script seems to add very little overhead to the compilation process.

As far as I can see, the missing source code info shouldn't impact any of the functionality that depends on the WKT descriptors, but let me know if you have any concerns with this approach!

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01 🎉

Comparison is base (64ca255) 75.36% compared to head (c777e01) 75.38%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #42      +/-   ##
==========================================
+ Coverage   75.36%   75.38%   +0.01%     
==========================================
  Files          31       31              
  Lines        5253     5253              
==========================================
+ Hits         3959     3960       +1     
+ Misses       1294     1293       -1     
Impacted Files Coverage Δ
prost-reflect/src/reflect/wkt.rs 83.33% <ø> (ø)

... 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

andrewhickman commented Apr 27, 2023

Thanks for looking into it, I do agree that removing source code info shouldn't be an issue, and we could always add it back behind a feature flag in future.

We could consider just modifying the file directly in source control rather than using a build script, but I'm not sure how the file was sourced or how it will be updated in the future. The build script seems to add very little overhead to the compilation process.

I think my prefered option would be to have a properly defined way to generate this file and then we can remove the source code info from the blob directly. I'll have a think about the best way to do that

EDIT: I had a bit of an experiment with adding a test for this purpose in #43 . There are a couple of annoying issues with using protox for this purpose since it has a dependency on prost-reflect itself

@srijs
Copy link
Contributor Author

srijs commented Apr 27, 2023

Makes sense, I like the idea of having a way to generate the file directly from the latest protobuf release.

Your experiment looks interesting, though I wonder if we should just keep things simple and use protoc to build the file?

@andrewhickman
Copy link
Owner

Closing in favour of #43

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