Skip to content

Separate immutable AST from mutable PreDict traversal layer for 2-4 parsing speedup - #1

Open
pevogam wants to merge 5 commits into
avocado-framework:mainfrom
pevogam:immutable-ast
Open

pevogam wants to merge 5 commits into
avocado-framework:mainfrom
pevogam:immutable-ast

Conversation

@pevogam

@pevogam pevogam commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

Keep each node's children behind Rc<Node> instead of Rc<RefCell<Node>>
no longer allowing mutation through a shared child reference. At the
same time store conditional nodes behind Rc handles instead of directly
so that cloning a handle shares existing syntax without copying the
entire conditional subtree.

Make process_content read-only and extract process_content_steps so
the same evaluation can operate on borrowed instruction sequences
(later via pre-dicts but already allowed here). It only requires
an iterable sequence of trackable lifetime borrowed content steps
and only copies the ones that are actually needed.

Adapt child insertion, conditional parsing, dumping, and traversal
to the new representation. Python boundary conversions and child
getters still return copied/detached Node values for inspection.

Signed-off-by: Plamen Dimitrov <plamen.dimitrov@intra2net.com>
PreDict walks parsed syntax and accumulates the state needed to produce
parameter dictionaries. This includes both failed cases as well as
partially processed content for the purpose of traversal. The former
is entirely migrated away from the now immutable nodes (part of the
overall immutable AST) and into PreDict which implements the actual
tree node traversal and allows multiple diverging traversals as separate
PreDict instances (pre-dicts). The latter separates a plain layer of
processed node content needed for multiple traversal-instance-bound
joining so that the pre-dict branch would consist of only borrowed
read-only nodes and not clone all their data.

The most important step to make these two possible is that pre-dicts
can now be updated from an immutable node. This allows the recursive
plain dict getter to only increase the reference count of a child node
and provide a borrowed version when updating the pre-dict from that
node. At the same time the join dict getter would increase the reference
count and only clone a join-free content together with each only filter.

More details about the failed cases:
- Failed cases use a type alias to reduce accumulated type complexity.
- The previous API of tiny methods regarding failed case manipulation
  has been reduced to a single "check_failed_case" method (previously
  "failed_case_might_pass") with the really small methods represented
  as direct implementations within the pre-dict node update method.
- Compared to processed node content failed cases should survive any
  pre-dict resets as the fields being popped describe the current
  traversal path.
- The "check_failed_case" method now accepts a similar content iterator
  and an explicit node content as well as failed case instance.

More details about the pre-dict update from a shared node:
- The borrowed node branch is only exposed externally as a clone for
  inspection purposes.
- The implementation of the new node update method has been slightly
  improved in terms of reusable ctx variables.
- The failed case now pushes a special "failed" frame that could then
  later be used by the traversal process so the parent can advance to
  the next child instead of retrying this one.

Both PreDict migrated functions were moved to rust-local scope and
thus need their own implementation block. The python side failed
cases test only focuses on empty dictionaries (python interface)
while a new rust side test contracts the actual pre-dict branch,
route, and collected failed cases.

Signed-off-by: Plamen Dimitrov <plamen.dimitrov@intra2net.com>
Add a frozen Tree object holding a shared root and expose it as an AST
attribute Parser.ast. Parsing additional configuration copies the root
value and returns a new tree snapshot while sharing existing descendants.
Earlier snapshots and dictionary traversals already in progress retain
their original syntax.

Note this means that the previous pattern of parse_file/parse_string
of replacing the parser node now correctly translated to a tree returning
another tree and makes sense since we can never truly guarantee a previous
tree is not already currently being traversed or held by another parser.
The tree struct remains immutable and thus safe and we can only return
a new (still guaranteed read-only) tree as a separate snapshot.

Each dictionary generator starts a fresh PreDict directly from the
currently held AST snapshot through a new PreDict.update_from_tree
facilitator. Tree.is_empty checks the root directly without copying
its children or content. The tree itself would only copy the root from
a previous snapshot to then use as a mutable node with the new filename
for both its own parse_string and parse_file replacing the module level
functions for the python side.

Parser.node becomes a getter for a detached inspection value and we
also test that modifying it does not affect the snapshot. The exposed
cloned node preserves the previous interface but also clarifies the
referential implementation of the tree structure (via a root node and
not via explicit ownership of all nodes).

The newly supported interleaved traversal and parsing are tested via
new contracts, inclusive of overall and fail case snapshot isolation.

Signed-off-by: Plamen Dimitrov <plamen.dimitrov@intra2net.com>
Iterate over the current internal and inherited content steps (or
instructions) by reference instead of cloning both sequences into
a temporary final-content vector and reusing that in hot paths. Most
importantly, use a borrowed final content iterator when processing
inherited content and when applying instructions to the final output
dictionary. The Python final_content getter still returns owned
inspection values when needed.

When it comes to the final output dictionary, we can also build name
and shortname from borrowed label strings rather than cloning
flattened label lists and then cloning each string. Token application
still clones the token it consumes but this is still much less
cloning than the previous approach.

Signed-off-by: Plamen Dimitrov <plamen.dimitrov@intra2net.com>
Entering the join dictionary getter of a pre-dict would previously
clone its entire PreDict instance, remove the current node, and
then take a shallow copy. This is already wasteful for both removing
a node that was just copied and for copying any unnecessary data.
Initialize the join selection directly from the already processed
parent frames instead, and reuse the same helper for a shallow copy.

The from_frames shallow copy mostly share the same implementation
with the exception of more direct calls and variable assignments.
We carry forward context, internal and inherited content, shortname,
dependencies and options, but start with fresh traversal and failure
history.

Include coverage for a test contract that the initial frame before
the first node pre-dict update also survives joins at the root as
well as for nested cases.

Signed-off-by: Plamen Dimitrov <plamen.dimitrov@intra2net.com>
@pevogam pevogam self-assigned this Sep 26, 2026
@pevogam pevogam added the enhancement New feature or request label Sep 26, 2026
@github-project-automation github-project-automation Bot moved this to Review Requested in Avocado Kanban Sep 26, 2026
@pevogam

pevogam commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator Author

@katusk This is the final implementation of the immutable AST concept and overall struct redesign! I would like to hear your rustacean opinion on the way this was done.

@katusk katusk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM at first glance, just few nitpicks.

Comment thread src/parser.rs
ContentType::Filters(Filters::JoinFilter {filter, line }) => {
// accumulate join steps
new_joins.push(ContentStep {
filename: t.filename.clone(),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You already .cloned() above, so probably you can just move the t.filename, right?

Comment thread src/parser.rs

// might pass if any filter (external or internal) is missing from all content
let in_all_content = |step: &ContentStep| {
content.clone().any(|candidate| candidate == step) || node_content.contains(step)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be a read-only operation, content does not need to be clone()d.

Comment thread src/parser.rs
match t.content_type {
ContentType::Filters(Filters::JoinFilter {filter, line }) => {
// accumulate join steps
new_joins.push(ContentStep {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'd add a ContentStep::new(...) constructor, that can be used at a bunch of different places instead of writing out all the field names -- which have different types afaics, so a new(...) should not make things more error-prone if you accidentally mix up the argument order.

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

Labels

enhancement New feature or request

Projects

Status: Review Requested

Development

Successfully merging this pull request may close these issues.

2 participants