Skip to content

Commit

Permalink
Don't allocate temporaries in FrameworkNameProvider.GetVersionString
Browse files Browse the repository at this point in the history 
Previously this would allocate a stack, enumerators, and a copy of the stack (for a call to Linq's `Reverse`).

With this change all state required to produce the version string is kept on the stack.

The unit test coverage for this method has been increased.

Also the allocation of an empty `Version` was removed in the `Try*` methods when they return false, to make them match their signatures and avoid a wasted allocation. Consuming code in this repo has been reviewed, and the behaviour captured in unit tests.
  • Loading branch information
@drewnoakes
drewnoakes committed Jun 20, 2023
1 parent a5bf9a8 commit 998ccd3
Show file tree
Hide file tree
Showing 2 changed files with 165 additions and 83 deletions.
139 changes: 76 additions & 63 deletions src/NuGet.Core/NuGet.Frameworks/FrameworkNameProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Diagnostics.CodeAnalysis;
using System.Globalization;
using System.Linq;
using System.Text;

namespace NuGet.Frameworks
{
Expand Down Expand Up @@ -132,11 +133,10 @@ public bool TryGetShortProfile(string frameworkIdentifier, string profile, [NotN

public bool TryGetVersion(string versionString, [NotNullWhen(true)] out Version? version)
{
version = null;

if (string.IsNullOrEmpty(versionString))
{
version = new Version(0, 0);
version = null;
return false;
}
else
{
Expand All @@ -159,17 +159,14 @@ public bool TryGetVersion(string versionString, [NotNullWhen(true)] out Version?
return Version.TryParse(string.Join(".", versionString.ToCharArray().Take(4)), out version);
}
}

return false;
}

public bool TryGetPlatformVersion(string versionString, [NotNullWhen(true)] out Version? version)
{
version = null;

if (string.IsNullOrEmpty(versionString))
{
version = new Version(0, 0);
version = null;
return false;
}
else
{
Expand All @@ -179,78 +176,94 @@ public bool TryGetPlatformVersion(string versionString, [NotNullWhen(true)] out
}
return Version.TryParse(versionString, out version);
}

return false;
}

public string GetVersionString(string framework, Version version)
{
var versionString = string.Empty;

if (version != null
&& (version.Major > 0
|| version.Minor > 0
|| version.Build > 0
|| version.Revision > 0))
if (version is null || IsZero(version))
{
var versionParts = new Stack<int>(4);
return string.Empty;
}

int major = version.Major > 0 ? version.Major : 0;
int minor = version.Minor > 0 ? version.Minor : 0;
int build = version.Build > 0 ? version.Build : 0;
int revision = version.Revision > 0 ? version.Revision : 0;

versionParts.Push(version.Major > 0 ? version.Major : 0);
versionParts.Push(version.Minor > 0 ? version.Minor : 0);
versionParts.Push(version.Build > 0 ? version.Build : 0);
versionParts.Push(version.Revision > 0 ? version.Revision : 0);
// remove all trailing zeros beyond the minor version
int partCount = (minor == 0, build == 0, revision == 0) switch
{
(true, true, true) => 1,
(false, true, true) => 2,
(_, false, true) => 3,
(_, _, false) => 4
};

if (partCount == 1)
{
// By default require the version to have 2 digits, for legacy frameworks 1 is allowed
var minPartCount = SingleDigitVersionFrameworks.Contains(framework) ? 1 : 2;

// remove all trailing zeros beyond the minor version
while ((versionParts.Count > minPartCount
&& versionParts.Peek() <= 0))
{
versionParts.Pop();
}
partCount = Math.Max(partCount, minPartCount);
}

// Always use decimals and 2+ digits for dotnet, netstandard, netstandardapp,
// netcoreapp, or if any parts of the version are over 9 we need to use decimals
if (string.Equals(
framework,
FrameworkConstants.FrameworkIdentifiers.NetCoreApp,
StringComparison.OrdinalIgnoreCase)
|| string.Equals(
framework,
FrameworkConstants.FrameworkIdentifiers.NetStandard,
StringComparison.OrdinalIgnoreCase)
|| versionParts.Any(x => x > 9)
|| string.Equals(
framework,
FrameworkConstants.FrameworkIdentifiers.NanoFramework,
StringComparison.OrdinalIgnoreCase))
{
// An additional zero is needed for decimals
if (versionParts.Count < 2)
{
versionParts.Push(0);
}
StringBuilder sb = StringBuilderPool.Shared.Rent(256);

versionString = string.Join(".", versionParts.Reverse());
}
else
{
versionString = string.Join(string.Empty, versionParts.Reverse());
}
// If the framework is netcoreapp, netstandard or nanoframework, or if any parts of the version are
// over 9 (requiring multiple digits), we need to add decimal points (periods) and have at least two parts.
if (StringComparer.OrdinalIgnoreCase.Equals(framework, FrameworkConstants.FrameworkIdentifiers.NetCoreApp) ||
StringComparer.OrdinalIgnoreCase.Equals(framework, FrameworkConstants.FrameworkIdentifiers.NetStandard) ||
StringComparer.OrdinalIgnoreCase.Equals(framework, FrameworkConstants.FrameworkIdentifiers.NanoFramework) ||
HasGreaterThanNinePart())
{
// An additional zero is needed for decimals
if (partCount == 1)
partCount = 2;

sb.Append(major);
if (partCount > 1)
sb.Append('.').Append(minor);
if (partCount > 2)
sb.Append('.').Append(build);
if (partCount > 3)
sb.Append('.').Append(revision);
}
else
{
sb.Append(major);
if (partCount > 1)
sb.Append(minor);
if (partCount > 2)
sb.Append(build);
if (partCount > 3)
sb.Append(revision);
}

return StringBuilderPool.Shared.ToStringAndReturn(sb);

bool HasGreaterThanNinePart()
{
return major > 9 || minor > 9 || build > 9 || revision > 9;
}

return versionString;
static bool IsZero(Version version)
{
// Build and Revision can be -1 when only major & minor are specified.
// Out of caution, check all values for zero or less.
return version.Major <= 0
&& version.Minor <= 0
&& version.Build <= 0
&& version.Revision <= 0;
}
}

// Legacy frameworks that are allowed to have a single digit for the version number
private static readonly HashSet<string> SingleDigitVersionFrameworks = new HashSet<string>(
new string[] {
FrameworkConstants.FrameworkIdentifiers.Windows,
FrameworkConstants.FrameworkIdentifiers.WindowsPhone,
FrameworkConstants.FrameworkIdentifiers.Silverlight
},
StringComparer.OrdinalIgnoreCase);
private static readonly HashSet<string> SingleDigitVersionFrameworks = new(StringComparer.OrdinalIgnoreCase)
{
FrameworkConstants.FrameworkIdentifiers.Windows,
FrameworkConstants.FrameworkIdentifiers.WindowsPhone,
FrameworkConstants.FrameworkIdentifiers.Silverlight
};

public bool TryGetPortableProfile(IEnumerable<NuGetFramework> supportedFrameworks, out int profileNumber)
{
Expand Down
109 changes: 89 additions & 20 deletions test/NuGet.Core.Tests/NuGet.Frameworks.Test/FrameworkNameProviderTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,30 +225,99 @@ public void FrameworkNameProvider_TryGetPortableFrameworks_RejectsInvalidPortabl
}

[Theory]
[InlineData("", "")]
[InlineData("1", "1")]
[InlineData("10", "1")]
[InlineData("100", "1")]
[InlineData("101", "101")]
[InlineData("1010", "101")]
[InlineData("1001", "1001")]
[InlineData("1.0", "1")]
[InlineData("1.0.0", "1")]
[InlineData("1.0.1", "101")]
[InlineData("1.0.1.0", "101")]
[InlineData("1.0.0.1", "1001")]
[InlineData("10.0", "10.0")]
[InlineData("10.1", "10.1")]
[InlineData("10.1.0.1", "10.1.0.1")]
[InlineData("1.1.10", "1.1.10")]
[InlineData("1.10.1", "1.10.1")]
public void FrameworkNameProvider_VersionRoundTrip(string versionString, string expected)
[InlineData(null)]
[InlineData("")]
[InlineData(" ")]
[InlineData("\t")]
[InlineData("Hello")]
public void TryGetVersion_Failures(string? versionString)
{
Assert.False(DefaultFrameworkNameProvider.Instance.TryGetVersion(versionString!, out Version? version));
Assert.Null(version);
}

[Fact]
public void GetVersionString_Null()
{
Assert.Equal(
string.Empty,
DefaultFrameworkNameProvider.Instance.GetVersionString(NetCoreApp, null!));
}

[Fact]
public void GetVersionString_Zero()
{
Assert.Equal(
string.Empty,
DefaultFrameworkNameProvider.Instance.GetVersionString(NetCoreApp, new Version(0, 0, 0, 0)));
Assert.Equal(
string.Empty,
DefaultFrameworkNameProvider.Instance.GetVersionString(NetCoreApp, new Version(0, 0, 0)));
Assert.Equal(
string.Empty,
DefaultFrameworkNameProvider.Instance.GetVersionString(NetCoreApp, new Version(0, 0)));
}

[Theory]
// NetCoreApp requires a minimum of two parts, and require decimal points
[InlineData(NetCoreApp, "1", "1.0")]
[InlineData(NetCoreApp, "10", "1.0")]
[InlineData(NetCoreApp, "100", "1.0")]
[InlineData(NetCoreApp, "101", "1.0.1")]
[InlineData(NetCoreApp, "1010", "1.0.1")]
[InlineData(NetCoreApp, "1001", "1.0.0.1")]
[InlineData(NetCoreApp, "1.0", "1.0")]
[InlineData(NetCoreApp, "1.0.0", "1.0")]
[InlineData(NetCoreApp, "1.0.1", "1.0.1")]
[InlineData(NetCoreApp, "1.0.1.0", "1.0.1")]
[InlineData(NetCoreApp, "1.0.0.1", "1.0.0.1")]
[InlineData(NetCoreApp, "10.0", "10.0")]
[InlineData(NetCoreApp, "10.1", "10.1")]
[InlineData(NetCoreApp, "10.1.0.1", "10.1.0.1")]
[InlineData(NetCoreApp, "1.1.10", "1.1.10")]
[InlineData(NetCoreApp, "1.10.1", "1.10.1")]
// AspNetCore has no special requirements
[InlineData(AspNetCore, "1", "10")]
[InlineData(AspNetCore, "10", "10")]
[InlineData(AspNetCore, "100", "10")]
[InlineData(AspNetCore, "101", "101")]
[InlineData(AspNetCore, "1010", "101")]
[InlineData(AspNetCore, "1001", "1001")]
[InlineData(AspNetCore, "1.0", "10")]
[InlineData(AspNetCore, "1.0.0", "10")]
[InlineData(AspNetCore, "1.0.1", "101")]
[InlineData(AspNetCore, "1.0.1.0", "101")]
[InlineData(AspNetCore, "1.0.0.1", "1001")]
[InlineData(AspNetCore, "10.0", "10.0")]
[InlineData(AspNetCore, "10.1", "10.1")]
[InlineData(AspNetCore, "10.1.0.1", "10.1.0.1")]
[InlineData(AspNetCore, "1.1.10", "1.1.10")]
[InlineData(AspNetCore, "1.10.1", "1.10.1")]
// WindowsPhone supports single digit versions
[InlineData(WindowsPhone, "1", "1")]
[InlineData(WindowsPhone, "10", "1")]
[InlineData(WindowsPhone, "100", "1")]
[InlineData(WindowsPhone, "101", "101")]
[InlineData(WindowsPhone, "1010", "101")]
[InlineData(WindowsPhone, "1001", "1001")]
[InlineData(WindowsPhone, "1.0", "1")]
[InlineData(WindowsPhone, "1.0.0", "1")]
[InlineData(WindowsPhone, "1.0.1", "101")]
[InlineData(WindowsPhone, "1.0.1.0", "101")]
[InlineData(WindowsPhone, "1.0.0.1", "1001")]
[InlineData(WindowsPhone, "10.0", "10.0")]
[InlineData(WindowsPhone, "10.1", "10.1")]
[InlineData(WindowsPhone, "10.1.0.1", "10.1.0.1")]
[InlineData(WindowsPhone, "1.1.10", "1.1.10")]
[InlineData(WindowsPhone, "1.10.1", "1.10.1")]
public void FrameworkNameProvider_VersionRoundTrip(string framework, string versionString, string expected)
{
var provider = DefaultFrameworkNameProvider.Instance;

provider.TryGetVersion(versionString, out Version? version);
Assert.True(provider.TryGetVersion(versionString, out Version? version));
Assert.NotNull(version);

string actual = provider.GetVersionString("Windows", version!);
string actual = provider.GetVersionString(framework, version);

Assert.Equal(expected, actual);
}
Expand Down

0 comments on commit 998ccd3

Please sign in to comment.