Fix performance issues in Add-Type #5243

iSazonov · 2017-10-26T13:57:35Z

Related #5158.

Fix description

Commit 1. Minor optimizations in OutputError().

Commit 2. Before the fix we read sources files twice in base.EndProcessing() and in this.EndProcessing() - it is excluded. Also now we read source files in StringBuilder to exclude large reallocations. Added one test.

Additional considerations

I expect the fix remove performance issues reported in #5158. So we can close the Issue by the PR and open new Issue to discuss refactoring the code to use CompileAssemblyFromFile(). Using Roslyn can open paths to enhance the Add-Type cmdlet's capabilities but requires a lot of work.

PaulHigin · 2017-10-26T16:52:34Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

                {
-                    this.sourceCode = "";
+                    StringBuilder sb = new StringBuilder(paths.Length);
+                    sourceCode = "";


sourceCode doesn't need to be initialized since it is always assigned after loop. Also we normally name class fields with beginning underscore: "_sourceCode".

I'll remove the line.

sourceCode is used in many places in the file. If I rename it will be very noisy and unrelated with the performance fix - I think it is better to leave it as is.

daxian-dbw · 2017-10-26T20:55:32Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

                    foreach (string file in paths)
                    {
-                        this.sourceCode += System.IO.File.ReadAllText(file) + "\n";
+                        sb.Append(System.IO.File.ReadAllText(file));


Some feedback:

ReadAllText returns a large string in the end, which may be allocated in LOH if the file size is larger than 85kb. The allocation of this large string will be wasted since we are appending it to the StringBuilder.
If using ReadAllLines, then only small strings will be created, which can be quickly garbage collected. So it probably would be better if we go with

foreach (string file in paths) { foreach (string line in File.ReadAllLines(file)) { sb.AppendLine(line); } } sourceCode = sb.ToString();

When there is only one file specified, which might be the case for the most time, this is still not performant. It could be better if we can treat the one-file case specially. Like:

if (paths.Length == 1) { sourceCode = File.ReadAllText(file); } else { var sb = new StringBuilder(paths.Length); foreach (string file in paths) { foreach (string line in File.ReadAllLines(file)) { sb.AppendLine(line); } } sourceCode = sb.ToString(); }

My thoughts was that we should get CompileAssemblyFromFile and there is no point in doing a lot of optimization. But we can get it for a while, so I'll add the optimization. Only it will work for small files - StringBuilder.MaxCapacity is int.MaxValue.

I don't understand your comment about CompileAssemblyFromFile (is it a class?). Could you please elaborate it a bit more? I quickly searched about CSharpSyntaxTree.ParseText, and didn't find a good way to make it consume multiple files all together directly from files. That API takes a SourceText type, which has a static method From that takes a file path, but not multiple files. But I'm sure there should be ways to do it, otherwise, how would a compiler work 😄.

For some background, the perf issue of Add-Type was reported by Azure Profiler team. They find this exact code causes a lot LOH allocations in some services and if we treat the path.length == 1 case specially, we can save, as quoted, "5.39 million samples here, or around 1% of total PowerShell.exe CPU cost". It was about the windows powershell, but this piece of code is same for powershell core.

didn't find a good way to make it consume multiple files all together directly from files

I didn't find too. So I decided to postpone this to a later PR.
Really CompileAssemblyFromFile is in CodeDom. There isn't such method In CodeAnalysis.
I found using SourceText.From.
Maybe @lzybkr know a right way.

daxian-dbw · 2017-10-26T20:59:41Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

                {
-                    this.sourceCode = "";
+                    StringBuilder sb = new StringBuilder(paths.Length);


Use paths.Length as the initial capacity is not very helpful. Maybe use a larger estimate?

PaulHigin · 2017-10-27T14:43:42Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

+
+                        foreach (string file in paths)
+                        {
+                            FileInfo f = new FileInfo(file);


I wonder what the cost is for obtaining file information. I assume the file doesn't need to be opened? Is the cost a reasonable trade off to StringBuilder automatic allocation?

I couldn't find out how it was implemented in CoreFX. 😕
In any case, if we want to handle a lot of files, we need to refactor the cmdlet so that we don't read all the files in one string, but compile file by file. I plan to do it later

PaulHigin · 2017-10-27T14:44:22Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

+                        {
+                            StringBuilder sb = new StringBuilder((int)initLength);
+
+                            foreach (string file in paths)


Please add a comment that this pattern is used to minimize potential temporary LOH allocations.

Comment added.

PaulHigin · 2017-10-27T14:49:04Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

+                        }
+                        else
+                        {
+                            sourceCode = "";


It was my understanding that ReadAllText() uses StringBuilder internally. Do we know that it correctly handles strings larger than MaxInt characters?

My bad. I removed the code.

daxian-dbw · 2017-10-27T19:24:09Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

+                        {
+                            FileInfo f = new FileInfo(file);
+                            initLength += f.Length;
+                        }


Calculate the total size might not be worth it. How about we just use a relatively large initial length? Say 8192 (8k)?
To be honest, I think if a user feed a file with more than 85k in size to Add-Type, they are using it wrongly (that Azure service might use Add-Type wrongly).

Could you ask Azure profile team about this? I guess they can have real things.

Do you mean asking about the azure service might misuse Add-Type? @PaulHigin already replied them and brought it up to them.

I meant what is typical file size they see in their traces?

If you mean the initial capacity for StringBuilder, I think there is no accurate answer for it as it's probably a case by case basis. The default initial capacity is 16 bytes, so I think as long as we are using a relatively large number, it should be OK. (it's micro optimization that may be premature anyway)

I dislike always pre-allocating StringBuilder to a large size. It may be over allocated for the majority of cases. But if we do this we should definitely ensure we pre-allocate at a size less than a LOH block. I like the idea of pre-allocating intelligently but this means computing the file sizes and I am concerned that may have a high cost (I seem to remember a .Net bug where FileInfo resulted in opening files on some platforms).

I am inclined to just leave it at the .Net default allocation. To me the big perf win was removing the duplicate file read / string allocation in the base class.

.Net default (16 bytes) is very small for file size. I think I agree that 8k is best in the case. Also we plan remove the code at all in follow PR so I ready put any value in the time :-)

PaulHigin · 2017-10-27T20:35:31Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs

                {
-                    this.sourceCode = "";
-                    foreach (string file in paths)
+                    if (paths.Length == 1)


Also I don't think we need to special case this for a single file. My understanding is that ReadAllText() uses StringBuilder internally in a similar way so there is no gain in using it over our StringBuilder use.

We plan remove the code at all in follow PR. We can keep it as is today, but I'll have that thought in follow PR.

My understanding is that ReadAllText() uses StringBuilder internally in a similar way so there is no gain in using it over our StringBuilder use.

The gain we get in specializing paths.length == 1 is to save a big string list that holds all lines and the big string array that is returned. See https://referencesource.microsoft.com/#mscorlib/system/io/file.cs,1018

After inspecting the API code, I start to suspect whether the ReadAllLines() API will actually save us from the LOH allocation because of the string list and the string array it converts to. It may double the LOH allocation ... Anyways, it's good that @iSazonov is going to eliminate the "reading text from file" code and instead try to compile the files directly.

iSazonov · 2017-10-31T15:00:01Z

@PaulHigin @daxian-dbw Thanks for review! It seems I found how compile files - I'll test and come back with new PR in days so you will have a choice that back port to PS 5.1.

daxian-dbw · 2017-10-31T15:13:11Z

@iSazonov Thanks for spending effort redesign the code, looking forward to the new PR!
As for porting to windows, one thing we all need to know is that in Windows powershell, add-type goes through completely different code path (it uses CodeDom in full .NET). So your new PR most likely won't be ported back, but this micro-optimization change may be ported back.

PaulHigin · 2017-10-31T17:16:56Z

@daxian-dbw, @iSazonov I just noticed that the AddTypeCommandBase abstract class is public. So removing the duplicate sourceCode read from there could potentially be a breaking change if someone was relying on that functionality. I feel it is an acceptable risk for Core 6 since it is unlikely anyone has a dependency on it. But for Windows (back porting) it could adversely affect a customer.

iSazonov · 2017-11-01T04:28:12Z

Good catch! We can avoid breaking change if we move AddTypeCommandBase.EndProcessing code in AddTypeCommand.EndProcessing. Makes a new PR?

PaulHigin · 2017-11-01T18:33:10Z

@iSazonov Yes, moving the EndProcessing code into the base class seems like the right thing to do.

Exclude ReadAllText from OutputError()

0e84b4d

iSazonov requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners October 26, 2017 13:57

iSazonov requested a review from lzybkr October 26, 2017 13:58

iSazonov self-assigned this Oct 26, 2017

iSazonov force-pushed the exclude-readalltext branch from 02a2a1d to fd56efe Compare October 26, 2017 14:38

Exclude double ReadAllText() from EndProcessing

db1d42d

iSazonov force-pushed the exclude-readalltext branch from fd56efe to db1d42d Compare October 26, 2017 14:57

PaulHigin suggested changes Oct 26, 2017

View reviewed changes

Remove sourceCode initialization

d0d4cc7

PaulHigin approved these changes Oct 26, 2017

View reviewed changes

daxian-dbw requested changes Oct 26, 2017

View reviewed changes

daxian-dbw reviewed Oct 26, 2017

View reviewed changes

Add optimizations

4af63f3

PaulHigin suggested changes Oct 27, 2017

View reviewed changes

daxian-dbw requested changes Oct 27, 2017

View reviewed changes

PaulHigin suggested changes Oct 27, 2017

View reviewed changes

PaulHigin approved these changes Oct 27, 2017

View reviewed changes

SteveL-MSFT added the Backport-5.1-Consider Consider to backport to Windows PowerShell 5.1 due to impact label Oct 28, 2017

iSazonov added 2 commits October 30, 2017 10:12

Add StringBuilder init size and remove unneeded code.

1386b8e

Set init size StringBuilder in 8192

8d8f6c2

iSazonov force-pushed the exclude-readalltext branch from fc1a6d9 to 8d8f6c2 Compare October 30, 2017 07:16

daxian-dbw approved these changes Oct 31, 2017

View reviewed changes

iSazonov merged commit 1c446c1 into PowerShell:master Oct 31, 2017

iSazonov deleted the exclude-readalltext branch October 31, 2017 15:00

iSazonov mentioned this pull request Nov 2, 2017

Fix breaking change in Add-Type #5306

Merged

daxian-dbw added the Breaking-Change breaking change that may affect users label Nov 3, 2017

iSazonov mentioned this pull request Nov 4, 2017

Add-Type ReadAllText perf improvement #5158

Closed

SteveL-MSFT removed the Backport-5.1-Consider Consider to backport to Windows PowerShell 5.1 due to impact label Feb 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix performance issues in Add-Type #5243

Fix performance issues in Add-Type #5243

iSazonov commented Oct 26, 2017

PaulHigin Oct 26, 2017

iSazonov Oct 26, 2017

daxian-dbw Oct 26, 2017 •

edited

Loading

iSazonov Oct 27, 2017

daxian-dbw Oct 27, 2017 •

edited

Loading

iSazonov Oct 27, 2017

daxian-dbw Oct 26, 2017

iSazonov Oct 27, 2017

PaulHigin Oct 27, 2017

iSazonov Oct 27, 2017

PaulHigin Oct 27, 2017

iSazonov Oct 27, 2017

PaulHigin Oct 27, 2017

iSazonov Oct 27, 2017

daxian-dbw Oct 27, 2017

iSazonov Oct 27, 2017

daxian-dbw Oct 27, 2017 •

edited

Loading

iSazonov Oct 27, 2017

daxian-dbw Oct 27, 2017

PaulHigin Oct 27, 2017

iSazonov Oct 27, 2017

PaulHigin Oct 27, 2017

iSazonov Oct 27, 2017

daxian-dbw Oct 31, 2017

iSazonov commented Oct 31, 2017

daxian-dbw commented Oct 31, 2017 •

edited

Loading

PaulHigin commented Oct 31, 2017

iSazonov commented Nov 1, 2017

PaulHigin commented Nov 1, 2017

Fix performance issues in Add-Type #5243

Fix performance issues in Add-Type #5243

Conversation

iSazonov commented Oct 26, 2017

Fix description

Additional considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Oct 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iSazonov commented Oct 31, 2017

daxian-dbw commented Oct 31, 2017 • edited Loading

PaulHigin commented Oct 31, 2017

iSazonov commented Nov 1, 2017

PaulHigin commented Nov 1, 2017

daxian-dbw Oct 26, 2017 •

edited

Loading

daxian-dbw Oct 27, 2017 •

edited

Loading

daxian-dbw Oct 27, 2017 •

edited

Loading

daxian-dbw commented Oct 31, 2017 •

edited

Loading