Add-Type ReadAllText perf improvement

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();

ReadAllLines put string in List then we put in StringBuilder - 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.

if ReadAllText is 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 use StreamReader.Read(char[] buffer, int index, int count) - this would minimize the number of large allocations to just 2 - the char[] 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?

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.

I'm surprised too that we compile a string instead of files. We could use CompileAssemblyFromFile

Good suggestion - it might not have been an option with CodeDom, but now that we're using Roslyn, there obviously should be better options.

Dongbo Wang (@daxian-dbw)

We need @powershell-committee conclusion - can we remove the public AddTypeCommandBase class (merge with AddTypeCommand)?

In #5243 we fixed a bug in Add-Type but added a breaking change. In #5306 we removed the breaking change restoring AddTypeCommandBase.

From Dongbo Wang (@daxian-dbw) comment:

I think the breaking changes to AddTypeCommandBase is most likely safe. I searched on Github and don't see the type AddTypeCommandBase gets used except AddType.
Given that, would it be OK that we just keep the breaking change made in #5243? Maybe you want to do further cleanup, such as completely remove the base type, in your subsequent work on Add-Type.

I don't see the need for AddTypeCommandBase too.

/cc Steve Lee (@SteveL-MSFT)

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.

@PowerShell/powershell-committee reviewed this and is ok with the changes already made to remove AddTypeCommandBase class

Navigation Menu

Repository navigation

Add-Type ReadAllText perf improvement #5158

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Add-Type ReadAllText perf improvement #5158

Description

Activity

iSazonov commented on Oct 19, 2017

lzybkr commented on Oct 20, 2017

PaulHigin commented on Oct 20, 2017

iSazonov commented on Oct 21, 2017

lzybkr commented on Oct 21, 2017

iSazonov commented on Nov 4, 2017

lzybkr commented on Nov 4, 2017

SteveL-MSFT commented on Jan 3, 2018

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions