Closed alex-kulakov closed 1 year ago
I still object to to so big changes.
I initiated https://github.com/DataObjects-NET/dataobjects-net/pull/242 to highlight potential improvements, which have to be done by separate PRs. And some of the changes were already merged into master
This PR is pretty much the same, well, I tried to keep it. The subset of changes which is already in master was merged here as well before any of my changes to be as accurate as possible. I'll update PR
What I did above original PR + merged subset
.ToList()
were either removed or replaced with .ToList(count)
extension;IndexBuilder
and other builders;TopologicalSorter
is able to return both IEnumerable<T>
and IReadOnlyList<T>
, the second is useful because capacity of result collection is known within the sorter and copying results that way is better than .ToList()
applied to IEnumerable<T>
.StringBuilder
usage within NameBuilder
was replaced because for three arguments it is a bit faster to use string interpolation (see benchmarks results below). Am I missing something?StringBuilder vs string interpolation benchmarks and results
[Benchmark]
public void StringBuilder() => _ = StringBuilderCase();
private string StringBuilderCase()
{
// emulates some workload before virtual index name composition
var prefix = GetPrefixString();
//emulates System.Type.Name property call
var typeName = GetTypeNameString();
var sb = new StringBuilder(prefix, prefix.Length + 12 + typeName.Length)
.Append(".FILTERED.")
.Append(typeName);
return ApplyNamingRules(sb.ToString());
}
[Benchmark]
public void Interpolation() => _ = InterpolationCase();
private string InterpolationCase()
{
var prefix = GetPrefixString();
var typeName = GetTypeNameString();
return ApplyNamingRules($"{prefix}.FILTERED.{typeName}");
}
[MethodImpl(MethodImplOptions.NoInlining)]
private string GetPrefixString() => "Base" + "Index" + "Name";
[MethodImpl(MethodImplOptions.NoInlining)]
private string GetTypeNameString() => "Some" + "Type" + "Name";
public string ApplyNamingRules(string name) =>
name.Replace('+', '.').Replace('[', '(').Replace(']', ')');
and the results
BenchmarkDotNet=v0.13.1, OS=Windows 10.0.19045 Intel Core i5-8400 CPU 2.80GHz (Coffee Lake), 1 CPU, 6 logical and 6 physical cores .NET SDK=6.0.400 [Host] : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT DefaultJob : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT
Method | Mean | Error | StdDev | Rank | Gen 0 | Allocated |
---|---|---|---|---|---|---|
StringBuilder | 75.34 ns | 0.491 ns | 0.459 ns | 2 | 0.0527 | 248 B |
Interpolation | 49.43 ns | 0.096 ns | 0.080 ns | 1 | 0.0204 | 96 B |
What I did above original PR + merged subset
- several
.ToList()
were either removed or replaced with.ToList(count)
extension;- a number of collections were initialized with known capacities to prevent resizes;
- added more static anonymous funcs within
IndexBuilder
and other builders;
Why not just make a brand new PR for these changes?
Because it fits the theme. And as the original PR was draft which required my assistance anyway, I decided to go a bit further. I don't chase PRs count increase :)
@SergeiPavlov I did some changes over #242, take a look. Your thoughts will be valuable, as you are original pull request submitter. I tried to measure certain parts with BenchmarkDotNet before applying them.