andrewhickman / prost-reflect

A protobuf library extending prost with reflection support and dynamic messages.
https://crates.io/crates/prost-reflect
Apache License 2.0
91 stars 21 forks source link

remove source code info from wkt bin #42

Closed srijs closed 1 year ago

srijs commented 1 year ago

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[bot] commented 1 year ago

Codecov Report

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

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

Additional details and impacted files ```diff @@ 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](https://codecov.io/gh/andrewhickman/prost-reflect/pull/42?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Hickman) | Coverage Δ | | |---|---|---| | [prost-reflect/src/reflect/wkt.rs](https://codecov.io/gh/andrewhickman/prost-reflect/pull/42?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Hickman#diff-cHJvc3QtcmVmbGVjdC9zcmMvcmVmbGVjdC93a3QucnM=) | `83.33% <ø> (ø)` | | ... and [2 files with indirect coverage changes](https://codecov.io/gh/andrewhickman/prost-reflect/pull/42/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrew+Hickman)

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

andrewhickman commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Closing in favour of #43