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.

Sign up for GitHub

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

Remove unnecessary remove from ProjectRootElementCache #8576

Merged
merged 4 commits into from
May 4, 2023
Merged

Remove unnecessary remove from ProjectRootElementCache #8576

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eaa2dd1
Remove unnecessary remove
Forgind Mar 17, 2023
742ad7a
PR comment
Forgind Mar 21, 2023
4b8d3b8
Include nulls
Forgind Mar 23, 2023
9bd1b66
Avoid NREs
Forgind Mar 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src/Build/Evaluation/ProjectRootElementCache.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,7 +460,6 @@ internal override void DiscardImplicitReferences()
}
else
{
_strongCache.Remove(rootElement);
RaiseProjectRootElementRemovedFromStrongCache(rootElement);
Copy link
Member

Choose a reason for hiding this comment

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

[nit / pre-existing issue] What guarantees that this PRE was in the old strong cache and is actually being removed? It looks like we also get here if it was a regular weak-only not-explicitly-loaded entry.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. There are multiple ways we could go down this path and erroneously call the remove event. I wanted to make this an unambiguous no-op, so I stopped short of removing the next call, which might be legitimate. If I were to rework it to be more correct, I think this is best:

if (rootElement.IsExplicitlyLoaded)
{
    if (oldStrongCache.Contains(rootElement))
    {
        _strongCache.AddFirst(rootElement);
    }
    else
    {
        // Has already been removed. We can consider adding it to the strong cache here...but it probably isn't worth it
    }
}
else
{
    if (oldStrongCache.Contains(rootElement))
    {
        RaiseProjectRootElementRemovedFromStrongCache(rootElement);
    }
    else
    {
        // Element was not in the strong cache and should not be added to it --> no op
    }
}

or more simply

if (oldStrongCache.Contains(rootElement))
{
    if (rootElement.IsExplicitlyLoaded)
    {
        _strongCache.AddFirst(rootElement);
    }
    else
    {
        RaiseProjectRootElementRemovedFromStrongCache(rootElement);
    }
}

I think either is better from a correctness perspective, but it means we'd always be calling Contains, which is a bit slow.

Copy link
Member

Choose a reason for hiding this comment

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

What if we make it iterate over the strong cache? Should be doable in one pass. And as a bonus, it would preserve the order, which the current implementation does not.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only issue I see with that is that we're also discarding implicit references in the weak cache in this pass, and since there can be ProjectRootElements in the weak cache but not the strong cache, we'd be deleting those from the weak cache whether they're explicitly loaded and not GC'd or not.

}
}
Expand Down