Skip to content

Commit

Permalink
[RAR] Stop removing from file cache just because an assembly is cache…
Browse files Browse the repository at this point in the history
…d in process (#6891)

Fixes #6844

### Context
RAR has several caches, including a process-wide cache and a (serialized) file cache. Previously, we had the process cache take priority (if it was up-to-date) and removed from the serialized cache, believing that meant some other process had taken "ownership" of serializing the assembly, but that doesn't actually make sense because that other process could be the same process currently looking at this assembly, and even if it were another process, that other process could have removed it from its own file cache. This removes that flawed logic so that we always write to the file cache any information we have. This improves incremental build times.

### Changes Made
Stopped removing from file cache when process-wide cache has information.

### Testing
Tried without this change and noted that the first few attempted cache lookups all missed. Tried with this change without killing processes and noted that they were cache hits; tried with this change after killing processes, and they were still cache hits.

Also noted that without this change, there were 0 things in the instanceLocalFileStateCache at the beginning of the first RAR execution. After it, there were 122.

All tests conducted with Ocelot repo.

### Notes
I actually mentioned that I didn't think it made sense to have this code here in the "questions" section of my RAR pre-caching document [here](https://microsoft-my.sharepoint.com/:w:/p/nmytelka/EfJOiPG6XPJMsDQ12Nk5-HIBm957qVk2DPB0R5XSpRx1RA?e=6bVaU6) but never looked into it more. Ah, well.
  • Loading branch information
Forgind committed Oct 13, 2021
1 parent 1f49f8a commit b3a99d5
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 15 deletions.
9 changes: 2 additions & 7 deletions src/Tasks/AssemblyDependency/ReferenceTable.cs
Expand Up @@ -741,13 +741,8 @@ ITaskItem referenceAssemblyName
/// </summary>
private static void TryConvertToAssemblyName(string itemSpec, string fusionName, ref AssemblyNameExtension assemblyName)
{
// FusionName is used if available.
string finalName = fusionName;
if (string.IsNullOrEmpty(finalName))
{
// Otherwise, its itemSpec.
finalName = itemSpec;
}
// FusionName is used if available; otherwise use itemspec.
string finalName = string.IsNullOrEmpty(fusionName) ? itemSpec : fusionName;

bool pathRooted = false;
try
Expand Down
11 changes: 3 additions & 8 deletions src/Tasks/SystemState.cs
Expand Up @@ -366,19 +366,14 @@ private FileState ComputeFileStateFromCachesAndDisk(string path)
// If the process-wide cache contains an up-to-date FileState, always use it
if (isProcessFileStateUpToDate)
{
// If a FileState already exists in this instance cache due to deserialization, remove it;
// another instance has taken responsibility for serialization, and keeping this would
// result in multiple instances serializing the same data to disk
if (isCachedInInstance)
// For the next build, we may be using a different process. Update the file cache.
if (!isInstanceFileStateUpToDate)
{
instanceLocalFileStateCache.Remove(path);
instanceLocalFileStateCache[path] = cachedProcessFileState;
isDirty = true;
}

return cachedProcessFileState;
}
// If the process-wide FileState is missing or out-of-date, this instance owns serialization;
// sync the process-wide cache and signal other instances to avoid data duplication
if (isInstanceFileStateUpToDate)
{
return s_processWideFileStateCache[path] = cachedInstanceFileState;
Expand Down

0 comments on commit b3a99d5

Please sign in to comment.