Optimize C++ generated code for structures with many conditional fields#241
Conversation
- Remove [[nodiscard]] annotations (C++17 feature) and create separate issue #242 to discuss C++14 vs C++17 support - Remove redundant check in ExpressionScope.add() that was already covered by the parent scope lookup - Revert unnecessary style change in _render_existence_test - Add comprehensive docstring to _generate_optimized_ok_method_body explaining the switch optimization with examples - Refactor switch/if generation to use templates for better readability - Optimize trivially-true conditions to skip if-block wrapper when condition is statically known to be true
Regenerate all golden .emb.h files to reflect changes from the optimized conditionals implementation.
Add golden file and test for many_conditionals.emb to ensure the generated C++ header is tracked for review.
EricRahm
left a comment
There was a problem hiding this comment.
Thanks for polishing this up! Looks really good, added some high level comments. I think overall it'd be a little easier to review if we split out the always true optimization.
| both guarded by tag == 0), only the first field for that case value is | ||
| checked in the switch. This is because once we've checked var_0 for case 0, | ||
| we break out of the switch. Fields with duplicate case values will fall | ||
| through to the if-statement path. |
There was a problem hiding this comment.
Overall great comment, I really appreciate the clear explanations. Sorry to harp on this, but can you add a test case to make sure the statements here match up with the code behavior?
I suspect this will lead to a bit more churn so I'll hold off on digging to much deeper.
| emb_file, | ||
| golden_file, | ||
| include_dirs=None, | ||
| compiler_flags=None, |
There was a problem hiding this comment.
The one_golden_test and run_one_golden_test changes seem like they might be a separate issue? If so would you mind splitting that out (and we can land it quickly).
|
|
||
| This module contains types used by the LR(1) parser, which are also used in | ||
| other parts of the compiler: | ||
| other parts of the compiler: |
There was a problem hiding this comment.
nit: this seems unrelated
| and group["expr"].type.boolean.value | ||
| ) | ||
|
|
||
| if is_always_true: |
There was a problem hiding this comment.
I love this change! I wonder if we could split it out out as a separate issue, I think it'd make the golden churn a little bit easier to grok and also highlight the impact of it :)
| const auto emboss_reserved_local_ok_subexpr_23 = ::emboss::support::And</**/bool, bool, bool, bool>(emboss_reserved_local_ok_subexpr_19, emboss_reserved_local_ok_subexpr_22); | ||
| const auto emboss_reserved_local_ok_subexpr_24 = ::emboss::support::LessThan</**/::std::int32_t, bool, ::std::int32_t, ::std::int32_t>(emboss_reserved_local_ok_subexpr_10, ::emboss::support::Maybe</**/::std::int32_t>(static_cast</**/::std::int32_t>(2LL))); | ||
| const auto emboss_reserved_local_ok_subexpr_25 = ::emboss::support::LessThan</**/::std::int32_t, bool, ::std::int32_t, ::std::int32_t>(emboss_reserved_local_ok_subexpr_3, ::emboss::support::Maybe</**/::std::int32_t>(static_cast</**/::std::int32_t>(4LL))); | ||
| const auto emboss_reserved_local_ok_subexpr_26 = ::emboss::support::Or</**/bool, bool, bool, bool>(emboss_reserved_local_ok_subexpr_24, emboss_reserved_local_ok_subexpr_25); |
There was a problem hiding this comment.
This feels like somehow this got worse, maybe the tradeoff is worth it but I wonder if we could do better.
| VALUE = static_cast</**/::std::int32_t>(10LL), | ||
|
|
||
| }; | ||
| template <class Enum> |
There was a problem hiding this comment.
I suspect this is a result of the fix for passing in compiler args in the one_golden_test changes?
| @@ -1,13 +0,0 @@ | |||
| # Copyright 2019 Google LLC | |||
There was a problem hiding this comment.
I don't think we want to delete this?
- Revert compiler_flags additions in one_golden_test.py (to be split out) - Restore testdata/__init__.py - Revert unrelated whitespace change in parser_types.py - Revert 'is_always_true' optimization to reduce golden churn - Add C++ test case `DuplicateCaseValueFallthrough` to verify switch behavior
8749630 to
d4cd926
Compare
Merge current master, which landed clang-format for the C++ back end (#262) and the optimized-conditionals work (#241). Both rewrote generated_code_templates and every testdata/golden_cpp/*.h. The template conflict is resolved by re-applying the JSON text-output logic on top of master's version, and all goldens are regenerated with clang-format 22.1.5. Fixes that turn the failing "Run Bazel tests (clang)" job green: - Remove internal-ism include paths from emboss_array_view_test.cc (testing/base/public/gunit.h, third_party/absl/..., third_party/ emboss/...). These do not resolve in the open-source build and broke compilation -- the same class of issue already corrected in the template. - Size JsonTestStruct's struct_array to two elements ([+40] -> [+8]) so the 87-byte struct becomes 55 bytes and fits the 57-byte test buffer. The oversized fixed array made WriteToTextStream read past the buffer (the tests assert exactly two elements), causing check failures. - Update the array JSON test expectation to compact form ("[-3,2,...]" rather than "[-3, 2, ...]") to match the compact JSON the implementation emits, resolving the test/implementation version skew noted in review. Additionally, emit newlines between fields in multiline JSON output so multiline JSON is properly formatted; the change is guarded so non-JSON text output is byte-for-byte unchanged. Adds a JsonMultilineOutput test.
This change improves the performance of the generated C++ code for Emboss structures containing many conditional fields.
Specifically, it optimizes the validation logic (e.g., the
Ok()method) and field access patterns where fields depend on a shared discriminant or tag. This results in faster runtime validation and reduced overhead when interacting with complex structures defined with extensive conditional logic.Performance Impact
Benchmarked on
testdata/many_conditionals.embwith 10,000 iterations x 100 tags, compiled with-c opt:Summary
Recreated from #235 after the source branch was deleted.