Skip to content

Remove extra cast to PSObject in ToStringEnumerable() method #10389

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

Closed
iSazonov wants to merge 5 commits into from
Closed

Remove extra cast to PSObject in ToStringEnumerable() method #10389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
dbf43ae
Remove extra cast to PSObject in ToStringEnumerable() method
iSazonov Aug 20, 2019
15bdd9e
Refactor return in ToStringEnumerator()
iSazonov Aug 22, 2019
8c72633
Add fast path for primitive types and strings
iSazonov Aug 23, 2019
88249e3
Fix ToStringEnumerator()
iSazonov Aug 27, 2019
db5848f
Add test for enumerable code path
iSazonov Aug 28, 2019
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
106 changes: 59 additions & 47 deletions src/System.Management.Automation/engine/MshObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1122,7 +1122,16 @@ internal static string ToStringEnumerator(ExecutionContext context, IEnumerator
while (enumerator.MoveNext())
{
object obj = enumerator.Current;
returnValue.Append(PSObject.ToString(context, obj, separator, format, formatProvider, false, false));
if (TryFastTrackPrimitiveTypes(obj, format, formatProvider, out string objString))
{
returnValue.Append(objString);
}
else
{
PSObject mshObj = PSObject.AsPSObject(obj);
returnValue.Append(PSObject.ToString(context, mshObj, separator, format, formatProvider, false, false));
}
Copy link
Member

@daxian-dbw daxian-dbw Aug 23, 2019

Choose a reason for hiding this comment

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

DateTime has a higher chance to get a custom ToString definition in ETS.
I think we should just strictly restrict the special handling to string and true primitive types (namely type.IsPrimitive is true).

var objType = obj.GetType();
if (objType != typeof(string) && !objType.IsPrimitive)
{
    obj = PSObject.AsPSObject(obj);
}

returnValue.Append(PSObject.ToString(context, obj, separator, format, formatProvider, false, false));
returnValue.Append(separatorToUse);

Copy link
Collaborator Author

@iSazonov iSazonov Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

What about Decimal? Also the PR intention is performance. In your sample we add more checks.

Then I think about new test and get:

 >1,$PSVersionTable,2 -join ";"

1;System.Management.Automation.PSVersionHashTable;2

>1,@(3,4),2 -join ";"

1;System.Object[];2

I would expect recursion to work. What do you thing?

Copy link
Member

@daxian-dbw daxian-dbw Aug 29, 2019

Choose a reason for hiding this comment

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

In my mind, a perf change shouldn't result in any behavioral changes, and that's why I'm concerned about you being aggressive here.
I would much prefer to be conservative

  • Only treat string specially and keep on wrapping other values into PSObject if we go for the most conservative manner;
  • Only treat IsPrimitive objects specially and keep wrapping other values if we go a bit less conservative

As for decimal, it's OK that we don't cover it. Again, it's not a functionality change that we should cover all scenarios, but a perf change that targets improve common scenarios.
As for IntPtr being a primitive type, I think it's fine too, as it's not really possible to use IntPtr directly in PowerShell script anyways.

My personal preference: only treat string specially. /cc @JamesWTruher @rjmholt what's your thoughts on this?

Copy link
Collaborator Author

@iSazonov iSazonov Aug 30, 2019

Choose a reason for hiding this comment

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

a perf change shouldn't result in any behavioral changes, and that's why I'm concerned about you being aggressive here

I am full agree and very appreciate your help!
The "aggressive" come from original issue where we consider primitive types too. I agree that for join operator strings are the most expected types for the operator. Nevertheless, I believe that there are many scenarios (like custom output formatting) when users will use the join operator in loops with primitive types. Notice that we change only an intermediate code and we added tests to exclude breaking change. So I believe we could be less conservative here. But I do not found how test the enumerator change and I'd revert the change.

Copy link
Collaborator

@rjmholt rjmholt Oct 24, 2019

Choose a reason for hiding this comment

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

/cc @JamesWTruher @rjmholt what's your thoughts on this?

I think specialising for string only makes the most sense. It's by far the most common, and the place where we don't break things. I don't think there are many people out there doing lots of string joins on other primitive types, but by breaking ETS we'd lose functionality


returnValue.Append(separatorToUse);
}

Expand All @@ -1131,9 +1140,7 @@ internal static string ToStringEnumerator(ExecutionContext context, IEnumerator
return string.Empty;
}

int separatorLength = separatorToUse.Length;
returnValue.Remove(returnValue.Length - separatorLength, separatorLength);
return returnValue.ToString();
return returnValue.ToString(0, returnValue.Length - separatorToUse.Length);
}

internal static string ToStringEnumerable(ExecutionContext context, IEnumerable enumerable, string separator, string format, IFormatProvider formatProvider)
Expand All @@ -1144,8 +1151,15 @@ internal static string ToStringEnumerable(ExecutionContext context, IEnumerable
{
if (obj != null)
{
PSObject mshObj = PSObject.AsPSObject(obj);
returnValue.Append(PSObject.ToString(context, mshObj, separator, format, formatProvider, false, false));
if (TryFastTrackPrimitiveTypes(obj, format, formatProvider, out string objString))
{
returnValue.Append(objString);
}
else
{
PSObject mshObj = PSObject.AsPSObject(obj);
returnValue.Append(PSObject.ToString(context, mshObj, separator, format, formatProvider, false, false));
}
}

returnValue.Append(separatorToUse);
Expand All @@ -1156,9 +1170,7 @@ internal static string ToStringEnumerable(ExecutionContext context, IEnumerable
return string.Empty;
}

int separatorLength = separatorToUse.Length;
returnValue.Remove(returnValue.Length - separatorLength, separatorLength);
return returnValue.ToString();
return returnValue.ToString(0, returnValue.Length - separatorToUse.Length);
}

private static string ToStringEmptyBaseObject(ExecutionContext context, PSObject mshObj, string separator, string format, IFormatProvider formatProvider)
Expand Down Expand Up @@ -1240,6 +1252,42 @@ internal static string ToStringParser(ExecutionContext context, object obj, IFor
}
}

private static bool TryFastTrackPrimitiveTypes(object value, string format, IFormatProvider formatProvider, out string str)
{
switch (Convert.GetTypeCode(value))
{
case TypeCode.String:
str = (string)value;
break;
case TypeCode.Byte:
case TypeCode.SByte:
case TypeCode.Int16:
case TypeCode.UInt16:
case TypeCode.Int32:
case TypeCode.UInt32:
case TypeCode.Int64:
case TypeCode.UInt64:
case TypeCode.DateTime:
case TypeCode.Decimal:
var formattable = (IFormattable)value;
str = formattable.ToString(format, formatProvider);
break;
case TypeCode.Double:
var dbl = (double)value;
str = dbl.ToString(format ?? LanguagePrimitives.DoublePrecision, formatProvider);
break;
case TypeCode.Single:
var sgl = (float)value;
str = sgl.ToString(format ?? LanguagePrimitives.SinglePrecision, formatProvider);
break;
default:
str = null;
return false;
}

return true;
}

/// <summary>
/// Called from an PSObject instance ToString to provide a string representation for an object.
/// </summary>
Expand Down Expand Up @@ -1269,42 +1317,6 @@ internal static string ToStringParser(ExecutionContext context, object obj, IFor
/// </exception>
internal static string ToString(ExecutionContext context, object obj, string separator, string format, IFormatProvider formatProvider, bool recurse, bool unravelEnumeratorOnRecurse)
{
bool TryFastTrackPrimitiveTypes(object value, out string str)
{
switch (Convert.GetTypeCode(value))
{
case TypeCode.String:
str = (string)value;
break;
case TypeCode.Byte:
case TypeCode.SByte:
case TypeCode.Int16:
case TypeCode.UInt16:
case TypeCode.Int32:
case TypeCode.UInt32:
case TypeCode.Int64:
case TypeCode.UInt64:
case TypeCode.DateTime:
case TypeCode.Decimal:
var formattable = (IFormattable)value;
str = formattable.ToString(format, formatProvider);
break;
case TypeCode.Double:
var dbl = (double)value;
str = dbl.ToString(format ?? LanguagePrimitives.DoublePrecision, formatProvider);
break;
case TypeCode.Single:
var sgl = (float)value;
str = sgl.ToString(format ?? LanguagePrimitives.SinglePrecision, formatProvider);
break;
default:
str = null;
return false;
}

return true;
}

PSObject mshObj = obj as PSObject;

#region plain object
Expand All @@ -1315,7 +1327,7 @@ bool TryFastTrackPrimitiveTypes(object value, out string str)
return string.Empty;
}

if (TryFastTrackPrimitiveTypes(obj, out string objString))
if (TryFastTrackPrimitiveTypes(obj, format, formatProvider, out string objString))
{
return objString;
}
Expand Down Expand Up @@ -1493,7 +1505,7 @@ bool TryFastTrackPrimitiveTypes(object value, out string str)
// we try the BaseObject's ToString
object baseObject = mshObj._immediateBaseObject;

if (TryFastTrackPrimitiveTypes(baseObject, out string baseObjString))
if (TryFastTrackPrimitiveTypes(baseObject, format, formatProvider, out string baseObjString))
{
return baseObjString;
}
Expand Down
32 changes: 32 additions & 0 deletions test/powershell/Language/Scripting/Join.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.

Describe 'Tests for join operator' -Tags "CI" {
BeforeAll {
}

It 'Join operator can use the custom ToString method defined in ETS' {
Add-Type -TypeDefinition @"
namespace TestNS
{
public class TestClass
{
public override string ToString()
{
return "1";
}
}
}
"@

$a1=[TestNS.TestClass]::new()
$a2=[TestNS.TestClass]::new()
$a3=[TestNS.TestClass]::new()

$a1,$a2,$a3 -join ";" | Should -BeExactly "1;1;1"

Update-TypeData -TypeName "TestNS.TestClass" -MemberType ScriptMethod -MemberName "ToString" -Value { return "test"}

$a1,$a2,$a3 -join ";" | Should -BeExactly "test;test;test"
}
}