Skip to content

Fix performance issues in Add-Type #5243

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iSazonov merged 6 commits into from
Oct 31, 2017
Merged

Fix performance issues in Add-Type #5243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0e84b4d
Exclude ReadAllText from OutputError()
iSazonov Oct 26, 2017
db1d42d
Exclude double ReadAllText() from EndProcessing
iSazonov Oct 26, 2017
d0d4cc7
Remove sourceCode initialization
iSazonov Oct 26, 2017
4af63f3
Add optimizations
iSazonov Oct 27, 2017
1386b8e
Add StringBuilder init size and remove unneeded code.
iSazonov Oct 27, 2017
8d8f6c2
Set init size StringBuilder in 8192
iSazonov Oct 30, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 38 additions & 38 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/AddType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -763,6 +763,14 @@ internal string GetUsingSet(Language language)
/// </summary>
protected override void EndProcessing()
{
// Prevent code compilation in ConstrainedLanguage mode
if (SessionState.LanguageMode == PSLanguageMode.ConstrainedLanguage)
{
ThrowTerminatingError(
new ErrorRecord(
new PSNotSupportedException(AddTypeStrings.CannotDefineNewType), "CannotDefineNewType", ErrorCategory.PermissionDenied, null));
}

// Generate an error if they've specified an output
// assembly type without an output assembly
if (String.IsNullOrEmpty(outputAssembly) && outputTypeSpecified)
Expand All @@ -779,36 +787,6 @@ protected override void EndProcessing()
ThrowTerminatingError(errorRecord);
return;
}

PopulateSource();
}

internal void PopulateSource()
{
// Prevent code compilation in ConstrainedLanguage mode
if (SessionState.LanguageMode == PSLanguageMode.ConstrainedLanguage)
{
ThrowTerminatingError(
new ErrorRecord(
new PSNotSupportedException(AddTypeStrings.CannotDefineNewType), "CannotDefineNewType", ErrorCategory.PermissionDenied, null));
}

// Load the source if they want to load from a file
if (String.Equals(ParameterSetName, "FromPath", StringComparison.OrdinalIgnoreCase) ||
String.Equals(ParameterSetName, "FromLiteralPath", StringComparison.OrdinalIgnoreCase)
)
{
sourceCode = "";
foreach (string file in paths)
{
sourceCode += System.IO.File.ReadAllText(file) + "\n";
}
}

if (String.Equals(ParameterSetName, "FromMember", StringComparison.OrdinalIgnoreCase))
{
sourceCode = GenerateTypeSource(typeNamespace, Name, sourceCode, language);
}
}

internal void HandleCompilerErrors(AddTypeCompilerError[] compilerErrors)
Expand Down Expand Up @@ -853,12 +831,12 @@ private void OutputError(AddTypeCompilerError error, string[] actualSource)
{
if (!String.IsNullOrEmpty(error.FileName))
{
actualSource = System.IO.File.ReadAllText(error.FileName).Split(Utils.Separators.Newline);
actualSource = System.IO.File.ReadAllLines(error.FileName);
}
}

string errorText = StringUtil.Format(AddTypeStrings.CompilationErrorFormat,
error.FileName, error.Line, error.ErrorText) + "\n";
error.FileName, error.Line, error.ErrorText) + Environment.NewLine;

for (int lineNumber = error.Line - 1; lineNumber < error.Line + 2; lineNumber++)
{
Expand All @@ -876,8 +854,8 @@ private void OutputError(AddTypeCompilerError error, string[] actualSource)

lineText += actualSource[lineNumber - 1];

errorText += "\n" + StringUtil.Format(AddTypeStrings.CompilationErrorFormat,
error.FileName, lineNumber, lineText) + "\n";
errorText += Environment.NewLine + StringUtil.Format(AddTypeStrings.CompilationErrorFormat,
error.FileName, lineNumber, lineText) + Environment.NewLine;
}
}

Expand Down Expand Up @@ -932,13 +910,35 @@ protected override void EndProcessing()
{
// Load the source if they want to load from a file
if (String.Equals(ParameterSetName, "FromPath", StringComparison.OrdinalIgnoreCase) ||
String.Equals(ParameterSetName, "FromLiteralPath", StringComparison.OrdinalIgnoreCase))
String.Equals(ParameterSetName, "FromLiteralPath", StringComparison.OrdinalIgnoreCase)
)
{
this.sourceCode = "";
foreach (string file in paths)
if (paths.Length == 1)
Copy link
Contributor

@PaulHigin PaulHigin Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@iSazonov iSazonov Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@daxian-dbw daxian-dbw Oct 31, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
this.sourceCode += System.IO.File.ReadAllText(file) + "\n";
sourceCode = File.ReadAllText(paths[0]);
}
else
{

// We replace 'ReadAllText' with 'StringBuilder' and 'ReadAllLines'
// to avoide temporary LOH allocations.

StringBuilder sb = new StringBuilder(8192);

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

sourceCode = sb.ToString();
}
}
else if (String.Equals(ParameterSetName, "FromMember", StringComparison.OrdinalIgnoreCase))
{
sourceCode = GenerateTypeSource(typeNamespace, Name, sourceCode, language);
}

CompileSourceToAssembly(this.sourceCode);
Expand Down
47 changes: 45 additions & 2 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/Add-Type.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,38 @@
$guid = [Guid]::NewGuid().ToString().Replace("-","")

Describe "Add-Type" -Tags "CI" {
BeforeAll {
$guid = [Guid]::NewGuid().ToString().Replace("-","")

$code1 = @"
namespace Test.AddType
{
public class BasicTest1
{
public static int Add1(int a, int b)
{
return (a + b);
}
}
}
"@
$code2 = @"
namespace Test.AddType
{
public class BasicTest2
{
public static int Add2(int a, int b)
{
return (a + b);
}
}
}
"@
$codeFile1 = Join-Path -Path $TestDrive -ChildPath "codeFile1.cs"
$codeFile2 = Join-Path -Path $TestDrive -ChildPath "codeFile2.cs"

Set-Content -Path $codeFile1 -Value $code1 -Force
Set-Content -Path $codeFile2 -Value $code2 -Force
}

It "Public 'Language' enumeration contains all members" {
[Enum]::GetNames("Microsoft.PowerShell.Commands.Language") -join "," | Should Be "CSharp,CSharpVersion7,CSharpVersion6,CSharpVersion5,CSharpVersion4,CSharpVersion3,CSharpVersion2,CSharpVersion1,VisualBasic,JScript"
}
Expand All @@ -20,4 +52,15 @@ public class AttributeTest$guid {}
It "Can load TPA assembly System.Runtime.Serialization.Primitives.dll" {
Add-Type -AssemblyName 'System.Runtime.Serialization.Primitives' -PassThru | Should Not Be $null
}

It "Can compile C# files" {

{ [Test.AddType.BasicTest1]::Add1(1, 2) } | Should Throw
{ [Test.AddType.BasicTest2]::Add2(3, 4) } | Should Throw

Add-Type -Path $codeFile1,$codeFile2

{ [Test.AddType.BasicTest1]::Add1(1, 2) } | Should Not Throw
{ [Test.AddType.BasicTest2]::Add2(3, 4) } | Should Not Throw
}
}