Skip to content

performance(kie-dmn-feel): Performance regression in Drools v10+ with DMN models - #6871

Open
yesamer wants to merge 10 commits into
apache:mainfrom
yesamer:apache-kie#6870
Open

performance(kie-dmn-feel): Performance regression in Drools v10+ with DMN models #6871
yesamer wants to merge 10 commits into
apache:mainfrom
yesamer:apache-kie#6870

Conversation

@yesamer

@yesamer yesamer commented Aug 4, 2026

Copy link
Copy Markdown
Member

Closes #6870

Fix significant performance regression in matches() and replace() FEEL functions (DMN)

Problem

After upgrading from 8.44.x to 10.x, users observed a ~60% increase in execution time on
large DMN workloads (e.g. 200 s → 315 s on 400k-row batches). The bottleneck was traced to
XQueryImplUtil, which is called on every invocation of the FEEL matches() and replace()
functions.

Two independent inefficiencies were introduced:

  1. new Processor(false) on every call (commit 8dac313): Saxon's Processor constructor
    initialises the entire Saxon configuration and performs a license check. Creating it per call
    cost ~60 ms per 2,000 rows on Saxon 12.x. Saxon's own documentation recommends one Processor
    instance per JVM.

  2. Multi-pass string escaping: escapeXmlCharactersReferencesForXPath() scanned the input
    string up to 11 times (one regex guard scan + one contains + one replace per special
    character), creating up to 5 intermediate String allocations per call. This is invoked 2–3
    times per matches()/replace() call, so the cost multiplied across the batch.

Changes

XQueryImplUtil.java

  • Extracted Processor and XQueryCompiler into private static final fields — one instance
    per JVM, shared across all calls. Both are thread-safe per Saxon's API contract.
  • Rewrote escapeXmlCharactersReferencesForXPath() as a single-pass StringBuilder loop with
    lazy allocation: no StringBuilder is created at all when the input contains no special
    characters (the common case in DMN), and the original String reference is returned unchanged.
    The Pattern field and java.util.regex import are no longer needed and have been removed.

XQueryImplUtilTest.java

  • Added 15 new test cases covering: explicit empty flags, multi-flag combinations (si, mi),
    all five XML special characters in input and pattern, special characters in replacement strings,
    and backreference syntax — ensuring the full escape→embed→evaluate pipeline is exercised
    end-to-end against Saxon.

Measured impact (Saxon 12.10, 2,000 rows/run, median of 5 runs)

Workload Original (main) Fixed Speedup
matches() — plain input 66 ms 9 ms 7.7×
matches() — XML special chars in input 67 ms 10 ms 6.9×
matches() — unique input per row 67 ms 9 ms 7.6×
replace() — plain input 86 ms 7 ms 12.8×
replace() — XML special chars in input 68 ms 10 ms 6.8×
replace() — unique input per row 65 ms 9 ms 7.7×

Extrapolated to a 400k-row batch: ~13–17 s → ~1–2 s per workload type.

We used a manual warm-up benchmark to measure the regression. The results show a consistent 7–12× improvement across all workload profiles. Results in production could be different.

@yesamer
yesamer requested a lite review from Copilot August 4, 2026 16:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Addresses a performance regression in DMN FEEL XQuery-based functions (Drools v10+) by reducing Saxon initialization overhead and strengthening regression coverage.

Changes:

  • Bumps Saxon-HE version to a newer release.
  • Reuses a single Saxon Processor / XQueryCompiler instead of re-creating them per evaluation.
  • Expands tests to cover flags and XML-special-character escaping paths for matches() / replace().

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

File Description
kie-parent/pom.xml Updates Saxon-HE version used across the build.
kie-dmn/kie-dmn-feel/src/main/java/org/kie/dmn/feel/util/XQueryImplUtil.java Reuses Saxon objects and rewrites XML escaping to a single-pass implementation.
kie-dmn/kie-dmn-feel/src/test/java/org/kie/dmn/feel/util/XQueryImplUtilTest.java Adds regression tests for flags and XML special characters in inputs/patterns/replacements.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yesamer
yesamer marked this pull request as draft August 4, 2026 17:05
@yesamer
yesamer marked this pull request as ready for review August 4, 2026 18:16
@yesamer
yesamer requested a review from tkobayas August 5, 2026 15:38
@tkobayas

tkobayas commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Filed #6876 for CI :: Build failure.

Comment on lines +45 to +46
* compilations. Since errors here are immediately wrapped and re-thrown as
* {@link IllegalArgumentException}, this is not a concern.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the error message could still be replaced by another thread in a concurrent scenario, even if it's wrapped in an IllegalArgumentException. So I think "this is not a concern" is too strong a statement.

We may instead acknowledge that we're using a shared instance for performance, with the trade-off that error messages may not always be attributed correctly.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tkobayas Thank you, I applied your suggestions and a Benchmark.

@tkobayas

tkobayas commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

The fix looks good, but we need to run CI before merging.

Btw, do we have a benchmark for the targeted FEEL operations? It would be greater if we can confirm the improvement (not mandatory).

@gitgabrio gitgabrio left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(removed)

@tiagobento

Copy link
Copy Markdown
Contributor

Re-triggered the PR checks after

was merged.

@tiagobento

Copy link
Copy Markdown
Contributor

@yesamer Just FYI, you don't need to merge the base branch with the PR branch anymore. Just re-running after things land on main is enough. We have a custom action now that simulates a "Squash and merge" for the PR, and it uses main at the moment the CI starts. That's the safest thing to do to prevent semantic conflicts before using Merge Queues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Performance regression in Drools v10+ with DMN models

5 participants