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

Use ReadOnlyMemory when calculating the related file extensions #5943

Merged
merged 7 commits into from
Aug 1, 2024
Merged

Use ReadOnlyMemory when calculating the related file extensions #5943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d88a6a3
Fun with benchmarks
nkolev92 Jul 31, 2024
7311108
Cleanup
nkolev92 Jul 31, 2024
c4cafc2
Cleanup
nkolev92 Jul 31, 2024
0cd6d8a
Cleanup
nkolev92 Jul 31, 2024
579cac7
bring the code up to date.
nkolev92 Jul 31, 2024
39b3833
Cleanup
nkolev92 Jul 31, 2024
de1360f
remove benchmark code
nkolev92 Jul 31, 2024
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
45 changes: 32 additions & 13 deletions src/NuGet.Core/NuGet.Packaging/ContentModel/ContentItemCollection.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,19 @@
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.IO;
using System.Linq;

namespace NuGet.ContentModel
{
public class ContentItemCollection
{
private static readonly ReadOnlyMemory<char> Dll = ".dll".AsMemory();
private static readonly ReadOnlyMemory<char> Exe = ".exe".AsMemory();
private static readonly ReadOnlyMemory<char> Winmd = ".winmd".AsMemory();

private List<Asset> _assets;
private ConcurrentDictionary<string, string> _assemblyRelatedExtensions;
private Dictionary<ReadOnlyMemory<char>, string> _assemblyRelatedExtensions;
Copy link
Contributor

Choose a reason for hiding this comment

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

@nkolev92 the SDK PR is now failing with this error in multiple tests, I wonder if it is related to this change:

C:\h\w\AADF0A48\p\d\sdk\9.0.100-ci\NuGet.targets(180,5): error : Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct.

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird, this codepath isn't run concurrently.


/// <summary>
/// True if lib/contract exists
Expand All @@ -22,7 +24,7 @@ public class ContentItemCollection
public void Load(IEnumerable<string> paths)
{
// Cache for assembly and it's related file extensions.
_assemblyRelatedExtensions = new ConcurrentDictionary<string, string>();
_assemblyRelatedExtensions = new();

// Read already loaded assets
_assets = new List<Asset>();
Expand Down Expand Up @@ -50,6 +52,7 @@ public void Load(IEnumerable<string> paths)
}
}

[Obsolete("Unused and will be removed in a future version.")]
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems to be used in the dotnet/sdk: dotnet/sdk#42458 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

2 options for you.

  • Supress the warning
  • Move to PopulateItemGroups

I'll revert the obsolete attribute for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

#5946

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks. I think moving to PopulateItemGroups is the right approach going forward.

public IEnumerable<ContentItem> FindItems(PatternSet definition)
{
return FindItemsImplementation(definition, _assets);
Expand Down Expand Up @@ -271,7 +274,7 @@ private List<ContentItem> FindItemsImplementation(PatternSet definition, IEnumer
internal string GetRelatedFileExtensionProperty(string assemblyPath, IEnumerable<Asset> assets)
{
//E.g. if path is "lib/net472/A.B.C.dll", the prefix will be "lib/net472/A.B.C."
string assemblyPrefix = assemblyPath.Substring(0, assemblyPath.LastIndexOf('.') + 1);
ReadOnlyMemory<char> assemblyPrefix = assemblyPath.AsMemory(0, assemblyPath.LastIndexOf('.') + 1);

if (_assemblyRelatedExtensions.TryGetValue(assemblyPrefix, out string relatedProperty))
{
Expand All @@ -283,16 +286,17 @@ internal string GetRelatedFileExtensionProperty(string assemblyPath, IEnumerable
{
if (asset.Path is not null)
{
string extension = Path.GetExtension(asset.Path);
if (extension != string.Empty &&
var extension = GetExtension(asset);

if (extension.Length > 0 &&
//Assembly properties are files with extensions ".dll", ".winmd", ".exe", see ManagedCodeConventions.
!extension.Equals(".dll", StringComparison.OrdinalIgnoreCase) &&
!extension.Equals(".exe", StringComparison.OrdinalIgnoreCase) &&
!extension.Equals(".winmd", StringComparison.OrdinalIgnoreCase) &&
!ReadOnlyMemoryEquals(extension, Dll) &&
!ReadOnlyMemoryEquals(extension, Exe) &&
!ReadOnlyMemoryEquals(extension, Winmd) &&
!asset.Path.Equals(assemblyPath, StringComparison.OrdinalIgnoreCase) &&
//The prefix should match exactly (case sensitive), as file names are case sensitive on certain OSes.
//E.g. for lib/net472/A.B.C.dll and lib/net472/a.b.c.xml, if we generate related property '.xml', the related file path is not predictable on case sensitive OSes.
asset.Path.StartsWith(assemblyPrefix, StringComparison.Ordinal))
asset.Path.AsMemory().Span.StartsWith(assemblyPrefix.Span, StringComparison.Ordinal))
{
if (relatedFileExtensionList is null)
{
Expand All @@ -306,16 +310,31 @@ internal string GetRelatedFileExtensionProperty(string assemblyPath, IEnumerable
// If no related files found.
if (relatedFileExtensionList is null || relatedFileExtensionList.Count == 0)
{
_assemblyRelatedExtensions.TryAdd(assemblyPrefix, null);
_assemblyRelatedExtensions[assemblyPrefix] = null;
return null;
}
else
{
relatedFileExtensionList.Sort();
string relatedFileExtensionsProperty = string.Join(";", relatedFileExtensionList);
_assemblyRelatedExtensions.TryAdd(assemblyPrefix, relatedFileExtensionsProperty);
_assemblyRelatedExtensions[assemblyPrefix] = relatedFileExtensionsProperty;
return relatedFileExtensionsProperty;
}

static bool ReadOnlyMemoryEquals(ReadOnlyMemory<char> x, ReadOnlyMemory<char> y)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth making this an extension method that the whole repo can use?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I had merged before this.
I think we could eventually.
You were thinking a build/shared type of method?

{
return x.Span.Equals(y.Span, StringComparison.OrdinalIgnoreCase);
}

static ReadOnlyMemory<char> GetExtension(Asset asset)
{
int lastIndexOfDot = asset.Path.LastIndexOf('.');
if (lastIndexOfDot != -1)
{
return asset.Path.AsMemory(lastIndexOfDot);
}
return default;
}
}

/// <summary>
Expand Down
4 changes: 3 additions & 1 deletion test/NuGet.Core.Tests/NuGet.Packaging.Test/RelatedFileExtensionPropertyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ public class RelatedFileExtensionPropertyTests
"lib/net50/system.exe",
"lib/net50/system.winmd" }, null)]
[InlineData("lib/net50/system.dll", new string[] { "lib/net50/system.dll",
"lib/net50/system.Core.dll" }, null)]
"lib/net50/system.Core.dll" }, null)]
[InlineData("lib/net50/system.dll", new string[] { "lib/net50/system.dll",
"lib/net50/system.EXE",
"lib/net50/system.Core.DLL"}, null)]
Expand All @@ -33,6 +33,8 @@ public class RelatedFileExtensionPropertyTests
[InlineData("lib/net50/system.test.dll", new string[] { "lib/net50/system.test.dll",
"lib/net50/system.test.PDB",
"lib/net50/system.test.XML", }, ".PDB;.XML")]
[InlineData("lib/net50/system.test.dll", new string[] { "lib/net50/system.test.dll",
"lib/net50/noextension" }, null)]

public void GetRelatedFileExtensionProperty_SingleAssemblyAsset_GetNullProperty(string assembly, string[] assetsPaths, string expectedRelatedProperty)
{
Expand Down
Loading