Only record extra Evaluator data when needed (#1060)

This saves memory

Author: cdmihai

src/XMakeBuildEngine/Definition/Project.cs

@@ -61,7 +61,12 @@ public enum ProjectLoadSettings
         /// <summary>
         /// Throw an exception and stop the evaluation of a project if any circular imports are detected
         /// </summary>
-        RejectCircularImports = 4
+        RejectCircularImports = 4,
+
+        /// <summary>
+        /// Record the item elements that got evaluated
+        /// </summary>
+        RecordEvaluatedItemElements = 8
     }
 
     /// <summary>
@@ -1069,6 +1074,7 @@ public static string GetEvaluatedItemIncludeEscaped(ProjectItemDefinition item)
         /// </returns>
         public List<GlobResult> GetAllGlobs()
         {
+            ReevaluateForPostMortemAnalysisIfNecessary();
             return GetAllGlobs(_data.EvaluatedItemElements);
         }
 
@@ -1082,6 +1088,8 @@ public List<GlobResult> GetAllGlobs(string itemType)
             {
                 return new List<GlobResult>();
             }
+
+            ReevaluateForPostMortemAnalysisIfNecessary();
             return GetAllGlobs(GetItemElementsByType(_data.EvaluatedItemElements, itemType));
         }
 
@@ -1156,6 +1164,7 @@ private IEnumerable<GlobResult> GetAllGlobs(ProjectItemElement itemElement)
         /// </returns>
         public List<ProvenanceResult> GetItemProvenance(string itemToMatch)
         {
+            ReevaluateForPostMortemAnalysisIfNecessary();
             return GetItemProvenance(itemToMatch, _data.EvaluatedItemElements);
         }
 
@@ -1166,6 +1175,7 @@ public List<ProvenanceResult> GetItemProvenance(string itemToMatch)
         /// <param name="itemType">The item type to constrain the search in</param>
         public List<ProvenanceResult> GetItemProvenance(string itemToMatch, string itemType)
         {
+            ReevaluateForPostMortemAnalysisIfNecessary();
             return GetItemProvenance(itemToMatch, GetItemElementsByType(_data.EvaluatedItemElements, itemType));
         }
 
@@ -1184,11 +1194,30 @@ public List<ProvenanceResult> GetItemProvenance(ProjectItem item)
                 return new List<ProvenanceResult>();
             }
 
+            ReevaluateForPostMortemAnalysisIfNecessary();
             var itemElementsAbove = GetItemElementsThatMightAffectItem(_data.EvaluatedItemElements, item);
 
             return GetItemProvenance(item.EvaluatedInclude, itemElementsAbove);
         }
 
+        /// <summary>
+        /// Some project APIs need to do analysis that requires the Evaluator to record more data than usual as it evaluates.
+        /// This method checks if the Evaluator was run with the extra required settings and if not, does a re-evaluation.
+        /// If a re-evaluation was necessary, it saves this information so a next call does not re-evaluate.
+        /// 
+        /// Using this method avoids storing extra data in memory when its not needed.
+        /// </summary>
+        private void ReevaluateForPostMortemAnalysisIfNecessary()
+        {
+            if (_loadSettings.HasFlag(ProjectLoadSettings.RecordEvaluatedItemElements))
+            {
+                return;
+            }
+
+            _loadSettings = _loadSettings | ProjectLoadSettings.RecordEvaluatedItemElements;
+            Reevaluate(LoggingService, _loadSettings);
+        }
+
         private static IEnumerable<ProjectItemElement> GetItemElementsThatMightAffectItem(List<ProjectItemElement> evaluatedItemElements, ProjectItem item)
         {
             return evaluatedItemElements
@@ -2392,34 +2421,22 @@ private ProjectInstance CreateProjectInstance(ILoggingService loggingServiceForE
         /// Re-evaluates the project using the specified logging service.
         /// </summary>
         private void ReevaluateIfNecessary(ILoggingService loggingServiceForEvaluation)
+        {
+            ReevaluateIfNecessary(loggingServiceForEvaluation, _loadSettings);
+        }
+
+        /// <summary>
+        /// Re-evaluates the project using the specified logging service and load settings.
+        /// </summary>
+        private void ReevaluateIfNecessary(ILoggingService loggingServiceForEvaluation, ProjectLoadSettings loadSettings)
         {
             // We will skip the evaluation if the flag is set. This will give us better performance on scenarios
             // that we know we don't have to reevaluate. One example is project conversion bulk addfiles and set attributes. 
             if (!SkipEvaluation && !_projectCollection.SkipEvaluation && IsDirty)
             {
                 try
                 {
-                    Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.Evaluate(_data, _xml, _loadSettings, ProjectCollection.MaxNodeCount, ProjectCollection.EnvironmentProperties, loggingServiceForEvaluation, new ProjectItemFactory(this), _projectCollection as IToolsetProvider, _projectCollection.ProjectRootElementCache, s_buildEventContext, null /* no project instance for debugging */);
-
-                    // We have to do this after evaluation, because evaluation might have changed
-                    // the imports being pulled in.
-                    int highestXmlVersion = Xml.Version;
-
-                    if (_data.ImportClosure != null)
-                    {
-                        foreach (Triple<ProjectImportElement, ProjectRootElement, int> triple in _data.ImportClosure)
-                        {
-                            highestXmlVersion = (highestXmlVersion < triple.Third) ? triple.Third : highestXmlVersion;
-                        }
-                    }
-
-                    _explicitlyMarkedDirty = false;
-                    _evaluatedVersion = highestXmlVersion;
-                    _evaluatedToolsetCollectionVersion = ProjectCollection.ToolsetsVersion;
-                    _evaluationCounter = GetNextEvaluationCounter();
-                    _data.HasUnsavedChanges = false;
-
-                    ErrorUtilities.VerifyThrow(!IsDirty, "Should not be dirty now");
+                    Reevaluate(loggingServiceForEvaluation, loadSettings);
                 }
                 catch (InvalidProjectFileException ex)
                 {
@@ -2429,6 +2446,31 @@ private void ReevaluateIfNecessary(ILoggingService loggingServiceForEvaluation)
             }
         }
 
+        private void Reevaluate(ILoggingService loggingServiceForEvaluation, ProjectLoadSettings loadSettings)
+        {
+            Evaluator<ProjectProperty, ProjectItem, ProjectMetadata, ProjectItemDefinition>.Evaluate(_data, _xml, loadSettings, ProjectCollection.MaxNodeCount, ProjectCollection.EnvironmentProperties, loggingServiceForEvaluation, new ProjectItemFactory(this), _projectCollection as IToolsetProvider, _projectCollection.ProjectRootElementCache, s_buildEventContext, null /* no project instance for debugging */);
+
+            // We have to do this after evaluation, because evaluation might have changed
+            // the imports being pulled in.
+            int highestXmlVersion = Xml.Version;
+
+            if (_data.ImportClosure != null)
+            {
+                foreach (Triple<ProjectImportElement, ProjectRootElement, int> triple in _data.ImportClosure)
+                {
+                    highestXmlVersion = (highestXmlVersion < triple.Third) ? triple.Third : highestXmlVersion;
+                }
+            }
+
+            _explicitlyMarkedDirty = false;
+            _evaluatedVersion = highestXmlVersion;
+            _evaluatedToolsetCollectionVersion = ProjectCollection.ToolsetsVersion;
+            _evaluationCounter = GetNextEvaluationCounter();
+            _data.HasUnsavedChanges = false;
+
+            ErrorUtilities.VerifyThrow(!IsDirty, "Should not be dirty now");
+        }
+
         /// <summary>
         /// Common code for the constructors.
         /// Applies global properties that are on the collection.

src/XMakeBuildEngine/Evaluation/Evaluator.cs

@@ -1642,7 +1642,7 @@ private void EvaluateItemElement(bool itemGroupConditionResult, ProjectItemEleme
 
                 if (conditionResult)
                 {
-                    _data.EvaluatedItemElements.Add(itemElement);
+                    RecordEvaluatedItemElement(itemElement);
                 }
 
                 return;
@@ -1660,7 +1660,7 @@ private void EvaluateItemElement(bool itemGroupConditionResult, ProjectItemEleme
 
         private void EvaluateItemElementUpdate(ProjectItemElement itemElement)
         {
-            _data.EvaluatedItemElements.Add(itemElement);
+            RecordEvaluatedItemElement(itemElement);
 
             var expandedItemSet =
                 new HashSet<string>(
@@ -1734,7 +1734,7 @@ private void EvaluateItemElementInclude(bool itemGroupConditionResult, bool item
             // FINALLY: Add the items to the project
             if (itemConditionResult && itemGroupConditionResult)
             {
-                _data.EvaluatedItemElements.Add(itemElement);
+                RecordEvaluatedItemElement(itemElement);
 
                 foreach (I item in items)
                 {
@@ -2624,6 +2624,14 @@ private string GetCurrentDirectoryForConditionEvaluation(ProjectElement element)
             }
         }
 
+        private void RecordEvaluatedItemElement(ProjectItemElement itemElement)
+        {
+            if (_loadSettings.HasFlag(ProjectLoadSettings.RecordEvaluatedItemElements))
+            {
+                _data.EvaluatedItemElements.Add(itemElement);
+            }
+        }
+
         /// <summary>
         /// Throws InvalidProjectException because we failed to import a project which contained a ProjectImportSearchPath fall-back.
         /// <param name="searchPathMatch">MSBuildExtensionsPath reference kind found in the Project attribute of the Import element</param>

src/XMakeBuildEngine/UnitTestsPublicOM/Definition/Project_Tests.cs

@@ -2535,6 +2535,41 @@ public void GetItemProvenanceOnlyGlob()
             AssertProvenanceResult(expected, project, "2.foo");
         }
 
+        [Fact]
+        public void GetItemProvenanceShouldReturnTheSameResultsIfProjectIsReevaluated()
+        {
+            var projectContents =
+            @"<Project ToolsVersion='msbuilddefaulttoolsversion' DefaultTargets='Build' xmlns='msbuildnamespace'>
+                  <ItemGroup>
+                    <A Include=`*.foo`/>
+                    <B Include=`1.foo;2.foo` Exclude=`*.foo`/>
+                    <C Include=`2` Exclude=`*.bar`/>
+                  </ItemGroup>
+                </Project>
+            ";
+
+            var expected = new ProvenanceResultTupleList
+            {
+                Tuple.Create("A", Operation.Include, Provenance.Glob, 1),
+                Tuple.Create("B", Operation.Exclude, Provenance.Glob, 1)
+            };
+
+            // Create a project. The initial evaluation does not record the information needed for GetItemProvenance
+            var project = ObjectModelHelpers.CreateInMemoryProject(projectContents);
+
+            // Since GetItemProvenance does not have the required evaluator data (evaluated item elements), it internally reevaluates the project to collect it
+            var provenanceResult = project.GetItemProvenance("2.foo");
+            AssertProvenanceResult(expected, provenanceResult);
+
+            // Dirty the xml to force another reevaluation.
+            project.AddItem("new", "new value");
+            project.ReevaluateIfNecessary();
+
+            // Assert that provenance returns the same result and that no data duplication happened
+            provenanceResult = project.GetItemProvenance("2.foo");
+            AssertProvenanceResult(expected, provenanceResult);
+        }
+
         [Fact]
         public void GetItemProvenanceShouldHandleComplexGlobExclusion()
         {