Skip to content

Commit

Permalink
Consider XML cache valid if loaded from non modifiable file (#8526)
Browse files Browse the repository at this point in the history
Related to #8412

Context
During research of optimization opportunities for Evaluation I have found relatively cheap win.
Some time ago we have introduced optimized up to date check leveraging immutable file locations. See usage FileClassifier class for more details. This PR made ProjectRootElementCache use it too so cached XML file is considered valid if it was read from immutable location.

Changes Made
As stated above. Additionally I tried to slightly improve readability by eliminating stuff if nesting.

Testing
Locally.
Measure gain for Orchard null incremental build /m was ~21% of Evaluation CPU time.
  • Loading branch information
rokonec authored Mar 7, 2023
1 parent 19b6185 commit 1a6d753
Showing 1 changed file with 47 additions and 34 deletions.
81 changes: 47 additions & 34 deletions src/Build/Evaluation/ProjectRootElementCache.cs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Xml;
using Microsoft.Build.Collections;
using Microsoft.Build.Construction;
using Microsoft.Build.Framework;
using Microsoft.Build.Internal;
using Microsoft.Build.Shared;
using ErrorUtilities = Microsoft.Build.Shared.ErrorUtilities;
Expand Down Expand Up @@ -140,45 +141,57 @@ internal ProjectRootElementCache(bool autoReloadFromDisk, bool loadProjectsReadO
/// </summary>
private bool IsInvalidEntry(string projectFile, ProjectRootElement projectRootElement)
{
if (projectRootElement != null && _autoReloadFromDisk)
// When we do not _autoReloadFromDisk we expect that cached value is always valid.
// Usually lifespan of cache is expected to be build duration (process will terminate after build).
if (projectRootElement == null || !_autoReloadFromDisk)
{
FileInfo fileInfo = FileUtilities.GetFileInfoNoThrow(projectFile);
return false;
}

// If the file doesn't exist on disk, go ahead and use the cached version.
// It's an in-memory project that hasn't been saved yet.
if (fileInfo != null)
{
if (fileInfo.LastWriteTime != projectRootElement.LastWriteTimeWhenRead)
{
// File was changed on disk by external means. Cached version is no longer valid.
// We could throw here or ignore the problem, but it is a common and reasonable pattern to change a file
// externally and load a new project over it to see the new content. So we dump it from the cache
// to force a load from disk. There might then exist more than one ProjectRootElement with the same path,
// but clients ought not get themselves into such a state - and unless they save them to disk,
// it may not be a problem.
return true;
}
else if (s_сheckFileContent)
{
// QA tests run too fast for the timestamp check to work. This environment variable is for their
// use: it checks the file content as well as the timestamp. That's better than completely disabling
// the cache as we get test coverage of the rest of the cache code.
XmlDocument document = new XmlDocument();
document.PreserveWhitespace = projectRootElement.XmlDocument.PreserveWhitespace;
// If the project file is non modifiable, assume it is up to date and consider the cached value valid.
if (!Traits.Instance.EscapeHatches.AlwaysDoImmutableFilesUpToDateCheck && FileClassifier.Shared.IsNonModifiable(projectFile))
{
return false;
}

using (var xtr = XmlReaderExtension.Create(projectRootElement.FullPath, projectRootElement.ProjectRootElementCache.LoadProjectsReadOnly))
{
document.Load(xtr.Reader);
}
FileInfo fileInfo = FileUtilities.GetFileInfoNoThrow(projectFile);

string diskContent = document.OuterXml;
string cacheContent = projectRootElement.XmlDocument.OuterXml;
// If the file doesn't exist on disk, go ahead and use the cached version.
// It's an in-memory project that hasn't been saved yet.
if (fileInfo == null)
{
return false;
}

if (diskContent != cacheContent)
{
return true;
}
}
if (fileInfo.LastWriteTime != projectRootElement.LastWriteTimeWhenRead)
{
// File was changed on disk by external means. Cached version is no longer valid.
// We could throw here or ignore the problem, but it is a common and reasonable pattern to change a file
// externally and load a new project over it to see the new content. So we dump it from the cache
// to force a load from disk. There might then exist more than one ProjectRootElement with the same path,
// but clients ought not get themselves into such a state - and unless they save them to disk,
// it may not be a problem.
return true;
}
else if (s_сheckFileContent)
{
// QA tests run too fast for the timestamp check to work. This environment variable is for their
// use: it checks the file content as well as the timestamp. That's better than completely disabling
// the cache as we get test coverage of the rest of the cache code.
XmlDocument document = new XmlDocument();
document.PreserveWhitespace = projectRootElement.XmlDocument.PreserveWhitespace;

using (var xtr = XmlReaderExtension.Create(projectRootElement.FullPath, projectRootElement.ProjectRootElementCache.LoadProjectsReadOnly))
{
document.Load(xtr.Reader);
}

string diskContent = document.OuterXml;
string cacheContent = projectRootElement.XmlDocument.OuterXml;

if (diskContent != cacheContent)
{
return true;
}
}

Expand Down

0 comments on commit 1a6d753

Please sign in to comment.