Repository navigation
Add-Type ReadAllText perf improvement #5158
Description
Add-Type uses File.ReadAllText() API in a loop for reading in multiple source files. This can be non-performant for multiple reasons:
a. ReadAllText() uses a small buffer that increases IO calls.
b. It performs StringBuilder.ToString() internally which allocates a string that is not used, making possible LOH allocation and adding GC pressure.
Consider using a different pattern:
StringBuilder sourceCode = new StringBuilder();
foreach (string file in paths)
{
foreach (string line in File.ReadAllLines(file))
{
sourceCode.AppendLine(line);
}
}
String result = sourceCode.ToString();
Activity
iSazonov commentedon Oct 19, 2017
ReadAllLines put string in
Listthen we put inStringBuilder- I don't see benefits.I found tests. ReadAllText looks best compared to ReadAllLines if we want to read the entire file in a single string.
lzybkr commentedon Oct 20, 2017
if
ReadAllTextis inefficient, we should be fixing that, not working around it.The use case here though is a little different, we're reading multiple files into a single buffer.
With some unsafe code, one could preallocate a
char[]buffer big enough to hold all of the files, then useStreamReader.Read(char[] buffer, int index, int count)- this would minimize the number of large allocations to just 2 - thechar[]and then turning that into a string.I am curious where this came up though - if this was a perf problem, why not just compile the code with C# ahead of time?
PaulHigin commentedon Oct 20, 2017
This was reported internally based on ETW perf traces showing a lot of LOH allocations from Add-Type/ReadAllText. But I think you are right, calling Add-Type so often is unexpected and sounds like a possible design/implementation error.
iSazonov commentedon Oct 21, 2017
I'm surprised too that we compile a string instead of files. We could use
CompileAssemblyFromFilelzybkr commentedon Oct 21, 2017
Good suggestion - it might not have been an option with CodeDom, but now that we're using Roslyn, there obviously should be better options.
iSazonov commentedon Nov 4, 2017
We need @powershell-committee conclusion - can we remove the public
AddTypeCommandBaseclass (merge withAddTypeCommand)?In #5243 we fixed a bug in
Add-Typebut added a breaking change. In #5306 we removed the breaking change restoringAddTypeCommandBase.From Dongbo Wang (@daxian-dbw) comment:
I don't see the need for
AddTypeCommandBasetoo./cc Steve Lee (@SteveL-MSFT)
lzybkr commentedon Nov 4, 2017
I don't think we've typically viewed the public api surface of a cmdlet implementation as a real C# api, so I'm fine with changes like this.
There might be a rare exception, and if we find out we made a mistake, it's generally much easier to fix that these days, as compared to waiting multiple years between releases.
SteveL-MSFT commentedon Jan 3, 2018
@PowerShell/powershell-committee reviewed this and is ok with the changes already made to remove AddTypeCommandBase class