Repository navigation
perf(xslt): node paths without a sibling scan inside XPath functions - #372
Conversation
XPath functions that name a node by its path (generate-id(), key(), id(), document(), unparsed-entity-uri(), stylesheet function calls and the key index) built that path by scanning each ancestor's children until the node was found, so naming every child of a wide parent cost quadratic XPath work. - The functions borrow the projection's node maps for the evaluation and read a node's path from the recorded per-node positions: one charged step per level. Attribute and namespace leaves keep their names from the evaluator node, including the implicit xml binding. - key(), id() and unparsed-entity-uri() read only the logical document index, walking to the logical root without building a path. - The key index takes its entry paths from the semantic node directly. - The sibling-scanning builder is removed. An engine test doubles the children of a wide parent and requires the XPath work to stay below triple; unit tests check that every node kind's path resolves back to the same node and that a late child's path fits a depth-sized allowance. Closes #371
…s path The path length read to admit a path's allocation is passed to the fill instead of being counted again, so a function-side path takes two ancestor walks instead of three and a key entry two instead of four. The base-URI index now admits each path key's bytes before allocating it.
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 |
|---|---|
Path mapping and cost accounting src/xslt/xpath.rs |
ProjectedPaths resolves evaluator nodes to semantic paths and logical-document indexes. NodeMaps accepts precomputed path lengths, and key-index path construction charges for work and allocation. |
XPath function integration src/xslt/xpath.rs |
Evaluator registrations pass ProjectedPaths to functions including key(), id(), generate-id(), document(), and unparsed-entity lookups. Deferred stylesheet-function calls also use the mapping. |
Path and work-limit validation src/xslt/xpath/tests.rs, crates/xml-sec-xslt/tests/engine.rs |
Tests verify projected path round trips, function behavior, and work and allocation limits. A wide-document regression test compares XPath operation counts at two document widths. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Refactor · Severity of issue fixed: Medium
Sequence Diagram(s)
sequenceDiagram
participant XPathFunction
participant ProjectedPaths
participant NodeMaps
participant EvaluationContext
XPathFunction->>ProjectedPaths: Resolve evaluator node path
ProjectedPaths->>NodeMaps: Read projected node position
ProjectedPaths->>EvaluationContext: Charge path work and allocation
ProjectedPaths-->>XPathFunction: Return semantic path
Possibly related PRs
- structured-world/xml-sec#366: Introduces the dense projection tables and node mappings used by this change.
Merge Risk: 
ede12
-
No actionable issue is established for this change; proceed with normal CI checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 30.43% 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 names the main change: XPath node path construction inside XSLT functions now avoids the sibling scan. The raw summary says the projected-path mapping replaces the sibling-scanning evaluator… Linked Issues check The changes satisfy the coding requirements in issue #371.ProjectedPathsmaps evaluator nodes through projected slot tables and recorded positions. It handles the package root, documents container,…Out of Scope Changes check The changes stay within issue #371. The implementation updates XPath path mapping, logical document lookup, path allocation accounting, and related base-URI indexing. The added tests cover these chang…Full details: Title check
-
Explanation
The title names the main change: XPath node path construction inside XSLT functions now avoids the sibling scan. The raw summary says the projected-path mapping replaces the sibling-scanning evaluator-tree path builder, and the objectives describe the same change. The title follows the conventional commit form and is specific enough for a teammate scanning history.
Full details: Linked Issues check
-
Explanation
The changes satisfy the coding requirements in issue
#371.ProjectedPathsmaps evaluator nodes through projected slot tables and recorded positions. It handles the package root, documents container, ordinary nodes, attributes, namespaces, and the implicitxmlnamespace.document_indexwalks to the logical root for document-scoped functions. Path construction charges XPath work by depth and reserves allocation before construction. The sibling-scanning builder is removed. Unit tests cover node-kind path round trips and budget limits. The engine test checks that doubling a wide parent remains below triple the XPath work. The PR description also reports passing workspace, donor, and XMLDSig XSLT interop tests.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within issue
#371. The implementation updates XPath path mapping, logical document lookup, path allocation accounting, and related base-URI indexing. The added tests cover these changes and their budget behavior. No unrelated product behavior or unrelated files are changed.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 30.43% 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 23 functions across 2 files. (1 skipped: 1 too large.)
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.
Summary
Changes
generate-id(),document()and stylesheet function calls borrow the projected node maps for the evaluation and build a node's path with one charged step per level; attribute and namespace leaves keep their names from the evaluator node, including the implicitxmlbinding.key(),id()andunparsed-entity-uri()read only the logical document index by walking to the logical root.Testing
An engine test doubles the children of a wide parent and requires the XPath work to stay below triple (the sibling scan quadrupled it); unit tests check that every node kind's path resolves back to the same node. Workspace tests with all features (including the xmlsec1 and libxslt oracles), Clippy, formatting and the
xsltfeature profile pass on macOS; Linux and Windows run in CI.Closes #371
Summary by CodeRabbit