Skip to content
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

Cache node evaluation to reuse results within a run #811

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

apognu
Copy link

@apognu apognu commented Jan 30, 2025

The AST language is idempotent within a single run. We can assume that it is expected a node configured the same way returns the same result within a single run of a scenario.

This PR makes it so:

  • Two identical nodes executing at the same time would only be executed once (the result being shared with the second, waiting execution).
  • A node executing after an identical node finished executing will reuse its result without executing.

@apognu apognu added enhancement New feature or request go Pull requests that update Go code labels Jan 30, 2025
@apognu apognu self-assigned this Jan 30, 2025
@apognu apognu force-pushed the feat/ast-eval-deduplication branch 2 times, most recently from fc96036 to 4384164 Compare January 30, 2025 15:08
This will now cache rule evaluation between AST node of the same run. It prevents two equal AST nodes from executing at the same time, using the result for all concurrent instances. Also, it will use the cached result in subsequent instance of the same AST node.
@apognu apognu force-pushed the feat/ast-eval-deduplication branch from 4384164 to 752dc4d Compare January 30, 2025 15:14
@apognu apognu requested a review from Pascal-Delange January 30, 2025 15:23
Copy link
Contributor

@Pascal-Delange Pascal-Delange left a comment

Choose a reason for hiding this comment

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

nice work !

Children []EvaluationStats
}

func BuildEvaluationStats(root NodeEvaluation, parentCached bool) EvaluationStats {
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing how the main source of execution delay by is linked to computing aggregates today, I think this may be at the same time a bit overkill for generic nodes and lack of detailed breakdown for aggregates nodes (and perhaps tomorrow, for sanction check/api call nodes)

Copy link
Contributor

Choose a reason for hiding this comment

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

(we can keep it though, it's fine)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request go Pull requests that update Go code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants