Skip to content

memoize redundant references #37191

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

Closed
wants to merge 2 commits into from
Closed

memoize redundant references #37191

wants to merge 2 commits into from

Conversation

dsa0x
Copy link
Member

@dsa0x dsa0x commented Jun 2, 2025

Fixes #

Target Release

1.13.x

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@dsa0x dsa0x force-pushed the sams/memo-eval-ctx branch from e8ea6c3 to 06600b3 Compare June 2, 2025 09:37
@dsa0x dsa0x added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Jun 2, 2025
Comment on lines -278 to -285
// The reference set we are given has not been de-duped, and so there can
// be redundant requests in it for two reasons:
// - The same item is referenced multiple times
// - Both an item and that item's container are separately referenced.
// We will still visit every reference here and ask our data source for
// it, since that allows us to gather a full set of any errors and
// warnings, but once we've gathered all the data we'll then skip anything
// that's redundant in the process of populating our values map.
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this comment may no longer be relevant to the current implementation.

Today, we have the .StaticValidReferences call that validates all the static references, and the main loop here just retrieves things from the data source, and that retrieval is done solely based on the reference's subject, not the entire traversal.

Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
no-changelog-needed Add this to your PR if the change does not require a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant