-
Notifications
You must be signed in to change notification settings - Fork 172
Chained filters optimization #1274
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
base: master
Are you sure you want to change the base?
Conversation
- Add enableFilterChainOptimization config option to JinjavaConfig (defaults to false) - Modify ExtendedParser to conditionally use AstFilterChain based on config - Override shouldUseFilterChainOptimization() in EagerExtendedParser to always return false - Remove EagerAstFilterChain.java as eager mode now uses old nested method approach - Revert test files to expect old format for eager execution
- Add AstFilterChainPerformanceTest to verify optimization performance - Tests verify both approaches produce identical results - Includes manual benchmark test (ignored by default) for detailed comparison - Automated test verifies optimized version is faster than unoptimized (at least 5% improvement) - Uses nested property access (content.text) to better simulate real-world usage
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| private ExecutionMode executionMode = DefaultExecutionMode.instance(); | ||
| private LegacyOverrides legacyOverrides = LegacyOverrides.NONE; | ||
| private boolean enablePreciseDivideFilter = false; | ||
| private boolean enableFilterChainOptimization = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all the tests pass if this is set to true?
jasmith-hs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks cool!
| String filterKey = ExtendedParser.FILTER_PREFIX + spec.getName(); | ||
| interpreter.getContext().addResolvedValue(filterKey); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be tested for parity with regular filter parsing and evaluation
| builder.append('('); | ||
| for (int i = 0; i < params.getCardinality(); i++) { | ||
| if (i > 0) { | ||
| builder.append(", "); | ||
| } | ||
| params.getChild(i).appendStructure(builder, bindings); | ||
| } | ||
| builder.append(')'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| builder.append('('); | |
| for (int i = 0; i < params.getCardinality(); i++) { | |
| if (i > 0) { | |
| builder.append(", "); | |
| } | |
| params.getChild(i).appendStructure(builder, bindings); | |
| } | |
| builder.append(')'); | |
| params.appendStructure(builder, bindings); |
| * Run with: mvn test -Dtest=AstFilterChainPerformanceTest | ||
| * Or run the main() method directly for more detailed output. | ||
| */ | ||
| public class AstFilterChainPerformanceTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests should be made separate from performance tests
Summary
This PR introduces an optimization for chained filter expressions in Jinjava templates. Instead of creating deeply nested AST nodes for expressions like
{{ foo|filter1|filter2|filter3 }}, a flattenedAstFilterChainnode is used, which improves performance by reducing recursive call overhead.Key changes:
AstFilterChainclass that processes filters iteratively instead of recursivelyFilterSpecclass to represent individual filter specifications in the chainenableFilterChainOptimizationin JinjavaConfig (defaults to false)EagerExtendedParseroverrides the config to always use the traditional nested approachTest plan
AstFilterChainPerformanceTestto verify both approaches produce identical results