Optimize C++ generated code for structures with many conditional fields#235
Optimize C++ generated code for structures with many conditional fields#235AaronWebster wants to merge 2 commits intogoogle:masterfrom
Conversation
| include_dirs = [] | ||
| compiler_flags = [] | ||
| for arg in argv[5:]: | ||
| if arg.startswith("--import-dir="): |
There was a problem hiding this comment.
This isn't very robust but I don't think import-dir actually works as intended. I just filed #236 , maybe the flag should be removed since it isn't used in current tests.
AaronWebster
force-pushed
the
optimized-conditionals-v2-squashed
branch
from
5394d0e to
8ea51fb
Compare
EricRahm
left a comment
EricRahm
left a comment
There was a problem hiding this comment.
Just some initial thoughts, what you're doing seems pretty cool! It'd just help to document the intent a little more.
| ${enum_usings} | ||
|
|
||
| bool Ok() const { | ||
| [[nodiscard]] bool Ok() const { |
There was a problem hiding this comment.
I appreciate adding these annotations, but maybe we should pull them out into a separate issue and PR to discuss them in isolation. I believe it's a C++ 17 feature and we may need to hash out some details around supporting (or not) C++14.
| scope = scope._parent | ||
|
|
||
| # Not found, add to current scope | ||
| if subexpr not in self._subexpr_to_name: |
There was a problem hiding this comment.
nit: I don't think we need this check anymore (it's covered above).
| def _render_existence_test(field, ir, subexpressions=None): | ||
| return _render_expression(field.existence_condition, ir, subexpressions) | ||
| return _render_expression( | ||
| field.existence_condition, ir, subexpressions=subexpressions |
There was a problem hiding this comment.
Is this change actually doing anything?
| lines = [] | ||
| for key in ordered_keys: | ||
| group = groups[key] | ||
| if group["type"] == "switch": |
There was a problem hiding this comment.
In general we maybe want to use templates for the switch and if blocks, this is a little hard to follow.
| return None, None | ||
|
|
||
|
|
||
| def _generate_optimized_ok_method_body(fields, ir, subexpressions): |
There was a problem hiding this comment.
This should probably have a big comment explaining what it's intending to do. If I understand correctly it's trying to take something like this emboss def:
struct blah:
0 [+4] UInt tag
if tag == 0:
4 [+4] UInt var_0
if tag == 1:
4 [+4] UInt var_1
and emit
{
const auto emboss_reserved_switch_discrim = ...;
if (!emboss_resevered_switch_discrim.Known()) return false;
switch (emboss_reserved_switch_discrim.ValueOrDefault()) {
case 0: if (!var_0.Ok()) return false; break;
case 1: if (!var_1.Ok()) return false; break;
}I might be misunderstanding what we're doing (hence having a big ol comment would be helpful).
Does this work for something like:
struct blah:
0 [+4] UInt tag
if tag == 0:
4 [+4] UInt var_0
if tag == 1:
4 [+4] UInt var_1
if tag == 0:
8 [+4] UInt var_x
| const auto emboss_reserved_cond = ::emboss::support::Maybe</**/bool>(true); | ||
| if (!emboss_reserved_cond.Known()) return false; | ||
| if (emboss_reserved_cond.ValueOrDefault()) { | ||
| if (!x().Ok()) return false; |
There was a problem hiding this comment.
So we're no longer doing Known and ValueOrDefault checks here, I'm not sure if that's intended or not. Is the idea that has_x(), has_IntrinsicSizeInBytes(), etc all boil down to ::emboss::support::Maybe</**/bool>(true); so we can lump them together?
- 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
3 participants
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 (on testdata/many_conditionals.emb with 10,000 iterations x 100 tags):
Runtime Speedup: ~8.7x faster (from ~1.83s to ~0.21s).
Binary Size Reduction: ~47% smaller (from 57848 bytes to 30472 bytes for many_conditionals_benchmark with -c opt).