Use string.Create instead of concatenation in PdhHelper#13425
Use string.Create instead of concatenation in PdhHelper#13425xtqqczze wants to merge 6 commits intoPowerShell:masterfrom
Conversation
This comment has been minimized.
This comment has been minimized.
|
Sorry, just realised I for some reason assumed the backing data structure was an array. Here's an updated benchmark: using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System;
using System.Collections.Generic;
using System.Collections.Specialized;
namespace CSharpPlayground
{
[MemoryDiagnoser]
public class LoopAllocationTest
{
public static void RunProfile()
{
BenchmarkRunner.Run<LoopAllocationTest>();
}
private const int Size = 1000000;
private string[] _stringArray;
private StringCollection _stringCollection;
private List<string> _stringList;
[GlobalSetup]
public void Setup()
{
_stringArray = new string[Size];
_stringCollection = new StringCollection();
_stringList = new List<string>();
var random = new Random();
for (int i = 0; i < Size; i++)
{
string s = random.NextDouble().ToString();
_stringArray[i] = s;
_stringCollection.Add(s);
_stringList.Add(s);
}
}
[Benchmark(Baseline = true)]
public void Run_For_Array()
{
for (int i = 0; i < _stringArray.Length; i++)
{
string s = _stringArray[i];
}
}
[Benchmark]
public void Run_ForEach_Array()
{
foreach (string s in _stringArray)
{
;
}
}
[Benchmark]
public void Run_For_StrCollection()
{
for (int i = 0; i < _stringCollection.Count; i++)
{
string s = _stringCollection[i];
}
}
[Benchmark]
public void Run_ForEach_StrCollection()
{
foreach (string s in _stringCollection)
{
;
}
}
[Benchmark]
public void Run_For_List()
{
for (int i = 0; i < _stringList.Count; i++)
{
string s = _stringList[i];
}
}
[Benchmark]
public void Run_ForEach_List()
{
foreach (string s in _stringList)
{
;
}
}
}
}Results: So it seems to be worth switching to |
This comment has been minimized.
This comment has been minimized.
|
And it is not hot path - I don't see a value from the PR. |
It's on an internal class, no? If it's exposed publicly, then best not to change it. But this being on a native helper class, it feels like we can make that change. |
I had assumed this method was a public API so I ignored the fact there are no references to |
Thanks for your help with benchmarking though, it is good to confirm |
@rjmholt Would there be value in a general PR to change method visibility from |
Generally I personally prefer |
| int capacity = blgFileNames.Count + 1; | ||
| for (int i = 0; i < blgFileNames.Count; i++) | ||
| { | ||
| capacity += blgFileNames[i].Length; | ||
| } | ||
|
|
||
| StringBuilder doubleNullTerminated = new StringBuilder(capacity); | ||
|
|
||
| for (int i = 0; i < blgFileNames.Count; i++) | ||
| { | ||
| doubleNullTerminated += fileName + '\0'; | ||
| string fileName = blgFileNames[i]; | ||
| doubleNullTerminated.Append(fileName).Append('\0'); | ||
| } | ||
|
|
||
| doubleNullTerminated += '\0'; | ||
| doubleNullTerminated.Append('\0'); |
There was a problem hiding this comment.
Given the way we calculate capacity up front here, it's relatively simple to move to string.Create():
string.Create(capacity, blgFileNames, (buf, fileNames) =>
{
int offset = 0;
for (int i = 0; i < fileNames.Count; i++)
{
string currFileName = fileNames[i];
int currFileNameLength = currFileName.Length;
Span<char> currSpan = buf.Slice(offset, currFileNameLength);
currFileName.AsSpan().CopyTo(currSpan);
offset += currFileNameLength;
buf[offset] = '\0';
offset++;
}
buf[offset] = '\0';
});There's probably a bug or two in the above, but I think the general idea is there
There was a problem hiding this comment.
@xtqqczze let me know if you're interested in trying this out. It's probably not a huge win relatively speaking (compared, for example, to native interop marshalling), but still relevant for an allocation/performance improvement PR
There was a problem hiding this comment.
@rjmholt We can also refactor the following code doing the same thing:
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
rjmholt
added
Waiting on Author
ghost
removed
the
Waiting on Author
apply @rjmholt suggestion
xtqqczze
force-pushed
the
StringBuilder-ConnectToDataSource
branch
from
cd94fda to
0e363ee
Compare
|
@iSazonov Do you any suggestions for CodeFactor issues here? |
|
@xtqqczze I like the formatting. Please ignore these CodeFactor warnings. |
xtqqczze
changed the title
|
Do we have a test for this code path? Want to make sure we would identify a regression |
|
There appear to be no references to |
|
@rjmholt Maybe we could add tests and refactor double null-terminated string generation code to a |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
SteveL-MSFT
added
the
CL-CodeCleanup
|
@xtqqczze Please resolve merge conflicts. |
|
@xtqqczze Please resolve merge conflicts. |
|
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
|
The PdhHelper.cs is not in the compile so we should close the PR. |
iSazonov
added
the
Resolution-Won't Fix
Assuming this is true, I'll close the PR. Let me know if there's something else we should be doing here. |
4 participants
PR Summary
ConnectToDataSourcePR Context
#13410 (comment)
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.