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
Optimize RAR's GetReferenceItems method #5929
Changes from 10 commits
cfe147c
17015af
fec708e
ed2bc5c
2fc517f
1dc3bc5
77b1038
507214d
4945ad2
cf314ab
7077b0e
afe2cf0
9dccb26
147ef43
2901929
fd0a93d
74cf67c
8a45a46
2a8893c
04e72ed
2bb3210
ff501aa
2f23808
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2506,14 +2506,6 @@ internal void GetReferenceItems | |
out ITaskItem[] copyLocalFiles | ||
) | ||
{ | ||
primaryFiles = Array.Empty<ITaskItem>(); | ||
dependencyFiles = Array.Empty<ITaskItem>(); | ||
relatedFiles = Array.Empty<ITaskItem>(); | ||
satelliteFiles = Array.Empty<ITaskItem>(); | ||
serializationAssemblyFiles = Array.Empty<ITaskItem>(); | ||
scatterFiles = Array.Empty<ITaskItem>(); | ||
copyLocalFiles = Array.Empty<ITaskItem>(); | ||
|
||
var primaryItems = new List<ITaskItem>(); | ||
var dependencyItems = new List<ITaskItem>(); | ||
var relatedItems = new List<ITaskItem>(); | ||
|
@@ -2522,10 +2514,10 @@ internal void GetReferenceItems | |
var scatterItems = new List<ITaskItem>(); | ||
var copyLocalItems = new List<ITaskItem>(); | ||
|
||
foreach (AssemblyNameExtension assemblyName in References.Keys) | ||
foreach (KeyValuePair<AssemblyNameExtension, Reference> kvp in References) | ||
{ | ||
string fusionName = assemblyName.FullName; | ||
Reference reference = GetReference(assemblyName); | ||
AssemblyNameExtension assemblyName = kvp.Key; | ||
Reference reference = kvp.Value; | ||
|
||
// Conflict victims and badimages are filtered out. | ||
if (!reference.IsBadImage) | ||
|
@@ -2554,7 +2546,7 @@ internal void GetReferenceItems | |
|
||
if (reference.IsResolved) | ||
{ | ||
ITaskItem referenceItem = SetItemMetadata(relatedItems, satelliteItems, serializationAssemblyItems, scatterItems, fusionName, reference, assemblyName); | ||
ITaskItem referenceItem = SetItemMetadata(relatedItems, satelliteItems, serializationAssemblyItems, scatterItems, assemblyName.FullName, reference, assemblyName); | ||
|
||
if (reference.IsPrimary) | ||
{ | ||
|
@@ -2573,9 +2565,7 @@ internal void GetReferenceItems | |
} | ||
} | ||
|
||
primaryFiles = new ITaskItem[primaryItems.Count]; | ||
primaryItems.CopyTo(primaryFiles, 0); | ||
|
||
primaryFiles = primaryItems.ToArray(); | ||
dependencyFiles = dependencyItems.ToArray(); | ||
relatedFiles = relatedItems.ToArray(); | ||
satelliteFiles = satelliteItems.ToArray(); | ||
|
@@ -2601,22 +2591,12 @@ internal void GetReferenceItems | |
private ITaskItem SetItemMetadata(List<ITaskItem> relatedItems, List<ITaskItem> satelliteItems, List<ITaskItem> serializationAssemblyItems, List<ITaskItem> scatterItems, string fusionName, Reference reference, AssemblyNameExtension assemblyName) | ||
{ | ||
// Set up the main item. | ||
ITaskItem referenceItem = new TaskItem(); | ||
TaskItem referenceItem = new TaskItem(); | ||
referenceItem.ItemSpec = reference.FullPath; | ||
referenceItem.SetMetadata(ItemMetadataNames.resolvedFrom, reference.ResolvedSearchPath); | ||
|
||
// Set the CopyLocal metadata. | ||
if (reference.IsCopyLocal) | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.copyLocal, "true"); | ||
} | ||
else | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.copyLocal, "false"); | ||
} | ||
|
||
// Set the FusionName metadata. | ||
referenceItem.SetMetadata(ItemMetadataNames.fusionName, fusionName); | ||
referenceItem.SetMetadata(ItemMetadataNames.copyLocal, reference.IsCopyLocal.ToString()); | ||
|
||
// Set the Redist name metadata. | ||
if (!String.IsNullOrEmpty(reference.RedistName)) | ||
|
@@ -2637,57 +2617,11 @@ private ITaskItem SetItemMetadata(List<ITaskItem> relatedItems, List<ITaskItem> | |
referenceItem.SetMetadata(ItemMetadataNames.imageRuntime, reference.ImageRuntime); | ||
} | ||
|
||
if (reference.IsWinMDFile) | ||
// The redist root is "null" when there was no IsRedistRoot flag in the Redist XML | ||
// (or there was no redist XML at all for this item). | ||
if (reference.IsRedistRoot != null) | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.winMDFile, "true"); | ||
|
||
// The ImplementationAssembly is only set if the implementation file exits on disk | ||
if (reference.ImplementationAssembly != null) | ||
{ | ||
if (VerifyArchitectureOfImplementationDll(reference.ImplementationAssembly, reference.FullPath)) | ||
{ | ||
if (string.IsNullOrEmpty(referenceItem.GetMetadata(ItemMetadataNames.winmdImplmentationFile))) | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.winmdImplmentationFile, Path.GetFileName(reference.ImplementationAssembly)); | ||
} | ||
|
||
// Add the implementation item as a related file | ||
ITaskItem item = new TaskItem(reference.ImplementationAssembly); | ||
// Clone metadata. | ||
referenceItem.CopyMetadataTo(item); | ||
// Related files don't have a fusion name. | ||
item.SetMetadata(ItemMetadataNames.fusionName, ""); | ||
RemoveNonForwardableMetadata(item); | ||
|
||
// Add the related item. | ||
relatedItems.Add(item); | ||
} | ||
} | ||
|
||
if (reference.IsManagedWinMDFile) | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.winMDFileType, "Managed"); | ||
} | ||
else | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.winMDFileType, "Native"); | ||
} | ||
} | ||
|
||
// Set the IsRedistRoot metadata | ||
if (reference.IsRedistRoot == true) | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.isRedistRoot, "true"); | ||
} | ||
else if (reference.IsRedistRoot == false) | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.isRedistRoot, "false"); | ||
} | ||
else | ||
{ | ||
// This happens when the redist root is "null". This means there | ||
// was no IsRedistRoot flag in the Redist XML (or there was no | ||
// redist XML at all for this item). | ||
referenceItem.SetMetadata(ItemMetadataNames.isRedistRoot, reference.IsRedistRoot.ToString()); | ||
} | ||
|
||
// If there was a primary source item, then forward metadata from it. | ||
|
@@ -2716,14 +2650,14 @@ private ITaskItem SetItemMetadata(List<ITaskItem> relatedItems, List<ITaskItem> | |
} | ||
|
||
// If the item originally did not have the implementation file metadata then we do not want to get it from the set of primary source items | ||
// since the implementation file is something specific to the source item and not supposed to be propigated. | ||
// since the implementation file is something specific to the source item and not supposed to be propagated. | ||
if (!hasImplementationFile) | ||
{ | ||
referenceItem.RemoveMetadata(ItemMetadataNames.winmdImplmentationFile); | ||
} | ||
|
||
// If the item originally did not have the ImageRuntime metadata then we do not want to get it from the set of primary source items | ||
// since the ImageRuntime is something specific to the source item and not supposed to be propigated. | ||
// since the ImageRuntime is something specific to the source item and not supposed to be propagated. | ||
if (!hasImageRuntime) | ||
{ | ||
referenceItem.RemoveMetadata(ItemMetadataNames.imageRuntime); | ||
|
@@ -2746,59 +2680,61 @@ private ITaskItem SetItemMetadata(List<ITaskItem> relatedItems, List<ITaskItem> | |
referenceItem.SetMetadata(ItemMetadataNames.version, String.Empty); | ||
} | ||
|
||
// Unset fusionName so we don't have to unset it later. | ||
referenceItem.RemoveMetadata(ItemMetadataNames.fusionName); | ||
|
||
List<string> relatedFileExtensions = reference.GetRelatedFileExtensions(); | ||
List<string> satellites = reference.GetSatelliteFiles(); | ||
List<string> serializationAssemblyFiles = reference.GetSerializationAssemblyFiles(); | ||
string[] scatterFiles = reference.GetScatterFiles(); | ||
Comment on lines
+2789
to
+2792
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My gut feeling is that these will all be small enough that keeping them live all at once instead of in sequence won't be a memory problem. But do you have data on that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope. How would I get such data? Build OrchardCore and...somehow check whether something is being paged out? |
||
Dictionary<string, string> nonForwardableMetadata = null; | ||
if (relatedFileExtensions.Count > 0 || satellites.Count > 0 || serializationAssemblyFiles.Count > 0 || scatterFiles.Length > 0) | ||
{ | ||
// Unset non-forwardable metadata now so we don't have to do it for individual items. | ||
nonForwardableMetadata = RemoveNonForwardableMetadata(referenceItem); | ||
} | ||
|
||
// Now clone all properties onto the related files. | ||
foreach (string relatedFileExtension in reference.GetRelatedFileExtensions()) | ||
foreach (string relatedFileExtension in relatedFileExtensions) | ||
{ | ||
ITaskItem item = new TaskItem(reference.FullPathWithoutExtension + relatedFileExtension); | ||
// Clone metadata. | ||
referenceItem.CopyMetadataTo(item); | ||
// Related files don't have a fusion name. | ||
item.SetMetadata(ItemMetadataNames.fusionName, ""); | ||
RemoveNonForwardableMetadata(item); | ||
|
||
// Add the related item. | ||
relatedItems.Add(item); | ||
} | ||
|
||
// Set up the satellites. | ||
foreach (string satelliteFile in reference.GetSatelliteFiles()) | ||
foreach (string satelliteFile in satellites) | ||
{ | ||
ITaskItem item = new TaskItem(Path.Combine(reference.DirectoryName, satelliteFile)); | ||
// Clone metadata. | ||
referenceItem.CopyMetadataTo(item); | ||
// Set the destination directory. | ||
item.SetMetadata(ItemMetadataNames.destinationSubDirectory, FileUtilities.EnsureTrailingSlash(Path.GetDirectoryName(satelliteFile))); | ||
// Satellite files don't have a fusion name. | ||
item.SetMetadata(ItemMetadataNames.fusionName, ""); | ||
RemoveNonForwardableMetadata(item); | ||
|
||
// Add the satellite item. | ||
satelliteItems.Add(item); | ||
} | ||
|
||
// Set up the serialization assemblies | ||
foreach (string serializationAssemblyFile in reference.GetSerializationAssemblyFiles()) | ||
foreach (string serializationAssemblyFile in serializationAssemblyFiles) | ||
{ | ||
ITaskItem item = new TaskItem(Path.Combine(reference.DirectoryName, serializationAssemblyFile)); | ||
// Clone metadata. | ||
referenceItem.CopyMetadataTo(item); | ||
// serialization assemblies files don't have a fusion name. | ||
item.SetMetadata(ItemMetadataNames.fusionName, ""); | ||
RemoveNonForwardableMetadata(item); | ||
|
||
// Add the serialization assembly item. | ||
serializationAssemblyItems.Add(item); | ||
} | ||
|
||
// Set up the scatter files. | ||
foreach (string scatterFile in reference.GetScatterFiles()) | ||
foreach (string scatterFile in scatterFiles) | ||
{ | ||
ITaskItem item = new TaskItem(Path.Combine(reference.DirectoryName, scatterFile)); | ||
// Clone metadata. | ||
referenceItem.CopyMetadataTo(item); | ||
// We don't have a fusion name for scatter files. | ||
item.SetMetadata(ItemMetadataNames.fusionName, ""); | ||
RemoveNonForwardableMetadata(item); | ||
|
||
// Add the satellite item. | ||
scatterItems.Add(item); | ||
|
@@ -2818,6 +2754,50 @@ private ITaskItem SetItemMetadata(List<ITaskItem> relatedItems, List<ITaskItem> | |
} | ||
} | ||
|
||
if (reference.IsWinMDFile) | ||
{ | ||
// The ImplementationAssembly is only set if the implementation file exits on disk | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Typo |
||
if (reference.ImplementationAssembly != null) | ||
{ | ||
if (VerifyArchitectureOfImplementationDll(reference.ImplementationAssembly, reference.FullPath)) | ||
{ | ||
// Add the implementation item as a related file | ||
ITaskItem item = new TaskItem(reference.ImplementationAssembly); | ||
// Clone metadata. | ||
referenceItem.CopyMetadataTo(item); | ||
|
||
// Add the related item. | ||
relatedItems.Add(item); | ||
|
||
referenceItem.SetMetadata(ItemMetadataNames.winmdImplmentationFile, Path.GetFileName(reference.ImplementationAssembly)); | ||
nonForwardableMetadata?.Remove(ItemMetadataNames.winmdImplmentationFile); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These inverted remove-from-the-removal-list things definitely need a comment please. |
||
} | ||
} | ||
|
||
nonForwardableMetadata?.Remove(ItemMetadataNames.winMDFileType); | ||
if (reference.IsManagedWinMDFile) | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.winMDFileType, "Managed"); | ||
} | ||
else | ||
{ | ||
referenceItem.SetMetadata(ItemMetadataNames.winMDFileType, "Native"); | ||
} | ||
nonForwardableMetadata?.Remove(ItemMetadataNames.winMDFile); | ||
referenceItem.SetMetadata(ItemMetadataNames.winMDFile, "true"); | ||
} | ||
|
||
// Set the FusionName metadata properly. | ||
referenceItem.SetMetadata(ItemMetadataNames.fusionName, fusionName); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "properly" mean? In general, how do you know this is safe? Is the local variable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If any of reference.GetSourceItems() has a fusionName, or reference.PrimarySourceItem has one, fusionName may have been set to an invalid value at this point. We remove it before forwarding referenceItem's metadata to other items, but we still need to set it properly at the end, i.e., give it the right fusionName. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you mind clarifying what you meant about fusionName being guaranteed to be empty? Previously, fusionName was set to a value near the top of this method, that value was passed on to a variety of "relatedItems," "satelliteItems," etc., and then it was unset from all of those other items. Here, instead, I'm unsetting it prior to using fusionName for any of those, and only giving it its value at the bottom, so CopyMetadataTo automatically skips fusionName. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The meta comment is: "properly" is basically useless in a code comment. Comments describe what you want the code to do/why it's done, and of course you want to do things "properly". Instead, describe details. Here, you might have a comment that |
||
|
||
if (nonForwardableMetadata != null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment please |
||
{ | ||
foreach (KeyValuePair<string, string> kvp in nonForwardableMetadata) | ||
{ | ||
referenceItem.SetMetadata(kvp.Key, kvp.Value); | ||
} | ||
} | ||
|
||
return referenceItem; | ||
} | ||
|
||
|
@@ -2951,15 +2931,37 @@ internal static UInt16 ReadMachineTypeFromPEHeader(string dllPath) | |
/// <summary> | ||
/// Some metadata should not be forwarded between the parent and child items. | ||
/// </summary> | ||
private static void RemoveNonForwardableMetadata(ITaskItem item) | ||
private static Dictionary<string, string> RemoveNonForwardableMetadata(ITaskItem item) | ||
ladipro marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
item.RemoveMetadata(ItemMetadataNames.winmdImplmentationFile); | ||
item.RemoveMetadata(ItemMetadataNames.imageRuntime); | ||
item.RemoveMetadata(ItemMetadataNames.winMDFile); | ||
Dictionary<string, string> metadata = new Dictionary<string, string>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there a more descriptive name for this variable? |
||
string meta = item.GetMetadata(ItemMetadataNames.winmdImplmentationFile); | ||
if (!String.IsNullOrEmpty(meta)) | ||
{ | ||
metadata.Add(ItemMetadataNames.winmdImplmentationFile, meta); | ||
} | ||
meta = item.GetMetadata(ItemMetadataNames.imageRuntime); | ||
if (!String.IsNullOrEmpty(meta)) | ||
{ | ||
metadata.Add(ItemMetadataNames.imageRuntime, meta); | ||
} | ||
meta = item.GetMetadata(ItemMetadataNames.winMDFile); | ||
if (!String.IsNullOrEmpty(meta)) | ||
{ | ||
metadata.Add(ItemMetadataNames.winMDFile, meta); | ||
} | ||
if (!Traits.Instance.EscapeHatches.TargetPathForRelatedFiles) | ||
{ | ||
meta = item.GetMetadata(ItemMetadataNames.targetPath); | ||
if (!String.IsNullOrEmpty(meta)) | ||
{ | ||
metadata.Add(ItemMetadataNames.targetPath, meta); | ||
} | ||
item.RemoveMetadata(ItemMetadataNames.targetPath); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since this one has to be next to the add to the new list, can you move the others to be next to their equivalents too? |
||
} | ||
item.RemoveMetadata(ItemMetadataNames.winmdImplmentationFile); | ||
item.RemoveMetadata(ItemMetadataNames.imageRuntime); | ||
item.RemoveMetadata(ItemMetadataNames.winMDFile); | ||
return metadata; | ||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,7 @@ public sealed class TaskItem : | |
#if FEATURE_APPDOMAIN | ||
MarshalByRefObject, | ||
#endif | ||
ITaskItem, ITaskItem2 | ||
ITaskItem2 | ||
{ | ||
#region Member Data | ||
|
||
|
@@ -298,9 +298,14 @@ public void CopyMetadataTo(ITaskItem destinationItem) | |
if (_metadata != null) | ||
{ | ||
// Avoid a copy if we can | ||
if (destinationItem is TaskItem destinationAsTaskItem && destinationAsTaskItem.Metadata == null) | ||
if (destinationItem is TaskItem destinationAsTaskItem && destinationAsTaskItem.Metadata?.Count < _metadata?.Count) | ||
{ | ||
destinationAsTaskItem.Metadata = _metadata.Clone(); // Copy on write! | ||
CopyOnWriteDictionary<string> copiedMetadata = _metadata.Clone(); // Copy on write! | ||
foreach (KeyValuePair<string, string> entry in destinationAsTaskItem.Metadata) | ||
{ | ||
copiedMetadata[entry.Key] = entry.Value; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you clarify the purpose of this change? Is this to optimistically optimize the case where you are copying metadata values that exactly match the destination (in value and count) - in this case the foreach here will not cause a "copy" of the dictionary because it will just be writing the same values? If that's the case, why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was previously an optimization for if destinationAsTaskItem.Metadata was null, i.e., its count was 0, but there are also cases in which its count is small compared to _metadata?.Count. This is trying to improve that case, where there are a few metadata defined for destinationAsTaskItem but not as many as for the source. In that case, it's better to modify a clone of the source rather than modifying the destination directly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be escaping, as on line 331? I'm not sure, but you'll want to make sure there's a test. IIRC, everything is always escaped when it enters the MSBuild engine, and unescaped on the way out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is copying metadata from a TaskItem to a TaskItem, so its escaped status should be the same in both cases. Just copying it over without escaping or unescaping it makes sense to me. Line 331 is for ITaskItems that are not ITaskItem2s, and this will always be an ITaskItem2, so if some escaping/unescaping is warranted, it should be replacing entry.Value with destinationAsTaskItem.GetMetadataValueEscaped(entry.Key), but that's less performant, and I don't understand why that would be warranted. Doesn't mean I'm right, though. |
||
} | ||
destinationAsTaskItem.Metadata = copiedMetadata; | ||
} | ||
else | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Convert to ternary operator