introduce a NOP template for cc #533
Conversation
|
cc @akonradi @rodaine any thoughts on this? The general background here is that the validate code takes up a large mount of binary space in Envoy Mobile and we would like to be able to compile it out. There are various ways we can do this, but this seems like a pretty easy quick fix (hack) that we could use until we get the full template override solution? WDYT? |
|
My first inclination is to say "why not do this with templates in consuming code and let the linker prune?" but I assume that's approximately the "full template override solution". I don't love the idea of generating incorrect code but as a stop-gap this seems okay since it's opt-in. |
|
I think #531 is the right direction. But we need a stopgap given how hard it is to updates What do you all think about getting this stopgap in for the time being? |
I'm +1 for going with this for now as it addresses an immediate need and is pretty simple. Can we add some basic tests and docs about this in the README or wherever is applicable? |
@Reflejo if you can address these, happy to approve :D |
|
@rodaine Ok picking this up again. Where would you recommend to add tests for this? Unsurprisingly there is no precedence of a test that would need a different BAZEL invocation (We'd need to pass template-flavor=nop for that particular test). |
|
@rodaine thoughts? I haven't found an easy way on the current test setup to include a test with a different bazel invocation. I could wire up a bunch of bazel primitives only for testing, but is it worth it for a temporary solution? |
|
It's been a long time since I've messed with Bazel. Can we create a new, specific test target that just handles the nop case? The test case for those will just be 1) that it compiles, and 2) that it never returns a validation error. I don't think this'll fit into the existing tests, and probably will be more work than it's worth to wedge it in. |
|
@Reflejo what's the status of this? Do you want to pick this up again and get this in? |
Reflejo
force-pushed
the
fz/cc_nop
branch
2 times, most recently
from
7304c23 to
94534e3
Compare
Signed-off-by: Martin Conte Mac Donell <[email protected]>
Signed-off-by: Martin Conte Mac Donell <[email protected]>
Signed-off-by: Martin Conte Mac Donell <[email protected]>
This change updates pgv to a version that supports a "cc nop" language. With that we can make the validation functions a NOP that hopefuly gets inlined. This is the PGV change for reference: bufbuild/protoc-gen-validate#533 Size delta: -3.7 Mi
This change updates pgv to a version that supports a "cc nop" language. With that we can make the validation functions a NOP that hopefuly gets inlined. This is the PGV change for reference: bufbuild/protoc-gen-validate#533 Size delta: -3.7 Mi Signed-off-by: Martin Conte Mac Donell <[email protected]>
This change updates pgv to a version that supports a "cc nop" language. With that we can make the validation functions a NOP that hopefuly gets inlined. This is the PGV change for reference: bufbuild/protoc-gen-validate#533 Size delta: -3.7 Mi Signed-off-by: Martin Conte Mac Donell <[email protected]>
This change updates pgv to a version that supports a "cc nop" language. With that we can make the validation functions a NOP that hopefuly gets inlined. This is the PGV change for reference: bufbuild/protoc-gen-validate#533 Size delta: -3.7 Mi Signed-off-by: Martin Conte Mac Donell <[email protected]>
* size: compile releases optimizing for code size Size delta: -0.66 Mi * size: set default visibility to hidden The .dynstr section of the binary as well as the symbol table is pretty big for a shared library with almost no (intended to be) exported symbols. This change makes all symbols hidden by default. We should test and add to visible if some are missing. Size delta: -5.66Mi * size: override pgv validations with NOP functions This change updates pgv to a version that supports a "cc nop" language. With that we can make the validation functions a NOP that hopefuly gets inlined. This is the PGV change for reference: bufbuild/protoc-gen-validate#533 Size delta: -3.7 Mi Signed-off-by: Martin Conte Mac Donell <[email protected]>
This is a stopgap before we can land #531 and its corresponding rules_go change.
What we really trying to achieve in https://github.com/envoyproxy/envoy-mobile is to be able to compile out the protos validate logic which takes a ton of space. This change helps achieving that without any code modification on envoy.