Repository navigation
perf(xslt): run a 16384-item transform within default limits - #366
Conversation
A signed document of 16384 items under an XSLT transform was refused by the default policy: the evaluator source kept three path-keyed hash tables per node, located each node by a linear sibling scan and cloned the whole source tree before evaluation. - Replace the path-keyed node maps with dense tables indexed by node id (document rank, sibling position, projected slot); node paths are derived from the tree and reverse lookup is a slot read - Drop the separate large node-set projection walk; one projection path charges only the selected nodes - Add Stylesheet::execute_owned_reported so the XMLDSig transform hands its parsed input to the engine instead of having it cloned - Rewrite calibrated engine memory tests as bounds and comparisons that keep their purpose without pinning the removed tables - Add an integration test that signs and verifies the 16384-item document under the default policy Closes #313
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Project XPath nodes and derive paths src/xslt/xpath.rs, src/xslt/xpath/tests.rs, src/sxd_document/dom_no_unsafe.rs, src/sxd_document/raw_no_unsafe.rs |
Node mapping derives paths and document order from dense tables and projected-node slots instead of retaining forward, reverse, and order hash maps. Base-URI and ID indexing use derived paths. Tests cover path resolution, projection, node ordering, and work and memory charges. |
Prepare and execute owned sources src/xslt/runtime.rs, src/xslt/xpath.rs, src/xmldsig/transforms/xslt.rs, src/xmldsig/transforms/xslt/tests.rs |
Stylesheet execution supports borrowed and owned source documents through shared execution setup. The XMLDSig transform passes its source document to owned execution. Workspace tests measure input canonicalization separately and check the reported execution outcome. |
Check workspace and large-transform behavior crates/xml-sec-xslt/tests/engine.rs, tests/xslt_transform_integration.rs |
Memory tests compare usage across messages, task frames, sorting, caches, document depth, projection, and attributes. The integration test signs and verifies a transformed document containing 16,384 items and checks the serialized output. |
Estimated code review effort: 4 (Complex) | ~45 minutes
Change: Bug fix · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant XMLDSigTransform
participant Stylesheet
participant XPathEvaluator
XMLDSigTransform->>Stylesheet: Transfer owned source document
Stylesheet->>XPathEvaluator: Prepare and execute source
XPathEvaluator-->>Stylesheet: Return transform result and usage
Stylesheet-->>XMLDSigTransform: Return execution outcome
Possibly related PRs
- structured-world/xml-sec#260: Adds the reported XSLT execution path that this change extends with owned-source execution.
- structured-world/xml-sec#254: Adds the XMLDSig XSLT adapter that this change updates to transfer source ownership.
- structured-world/xml-sec#293: Changes the execution-options contract used by the owned execution path.
Merge Risk: 
3ec3a
-
No identified issue remains that should block merging after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 20.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: enabling a 16,384-item XSLT transform within the default resource limits. Linked Issues check Only active directly linked issue is [ #313]. The new integration test creates 16,384Itemelements, signs them withSigningPolicy::default(), verifies the signature, and checks the exact pre-diges…Out of Scope Changes check The changes stay within [ #313]. Dense slot and rank maps, direct node lookup, path accounting, selected-node projection, and owned source execution reduce or meter the XSLT costs identified by the iss…Full details: Linked Issues check
-
Explanation
Only active directly linked issue is [
#313]. The new integration test creates 16,384Itemelements, signs them withSigningPolicy::default(), verifies the signature, and checks the exact pre-digest output. Dense evaluator tables, selected-node projection, andexecute_owned_reportedaddress the reported workspace and work growth. The PR keeps the resource ceilings unchanged. Existing limit tests retain refusal coverage, and the author reports the XSLT, workspace, conformance, and oracle suites pass. The exact output assertion and reported suite results support byte-identical output requirements.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within [
#313]. Dense slot and rank maps, direct node lookup, path accounting, selected-node projection, and owned source execution reduce or meter the XSLT costs identified by the issue. The updated memory and limit tests verify those changes. The 16,384-item integration test verifies the required signing and verification behavior. No unrelated feature or policy change is shown.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 20.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 53 functions across 8 files.
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
Generate docstrings
-
- Commit to this branch
- Create a new PR
Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f6569fae1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Projection records each attribute's arena slot, so an evaluator attribute maps back to its owner and index without searching the owner's attributes; `@*` over N attributes no longer costs N^2 unmetered comparisons - Mapping a namespace node back, which has no arena slot, admits the search of the owner's declarations as XPath work before running it - The context node's path is allocated at its exact length and its heap bytes are charged to owned memory for the whole evaluation Regression tests count attribute comparisons, check the namespace charge, and require a deeper context to need more owned memory.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba94c2e521
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Locating a semantic node in the evaluator tree built its path, walked it from the root and searched the owner's attributes by name, unmetered: binding a node-set of N attributes, keying them or evaluating in their context cost N^2. Replaying stylesheet-function arguments from node paths searched attributes the same way. - Projection records each node's arena slot by node id, and the evaluator document returns the node in a slot directly; an attribute is taken at its source index, which projection preserves, after a name check - The context node is located the same way - Resolving a node path still names an attribute or namespace node, so the search of the owner's declarations is admitted as XPath work Regression tests count attribute comparisons for evaluator lookups and require path replay to charge the comparisons it makes.
Summary
Changes
Stylesheet::execute_owned_reportedtakes the source tree by value; the XMLDSig XSLT transform uses it, so the parsed input is not cloned.Testing
Workspace tests with all features (including the xmlsec1 and libxslt oracles), the XSLT engine suite, Clippy and the feature-profile lints pass on macOS; Linux, Windows, no-std and the remaining matrix run in CI.
Closes #313
Summary by CodeRabbit