Adding LanguagePrimitives.TryCompare to provide faster comparisons by powercode · Pull Request #7438 · PowerShell/PowerShell

powercode · 2018-08-03T00:27:29Z

PR Summary

This PR adds the TryCompare equivalent of Compare, just as we have a TryConvert alternative to Convert.
This enables faster comparison in the cases where the convertions throws exception.

As an example:

 1..10000 | foreach{$_; [timespan]::FromSeconds($_)}  | sort

takes 3 800 ms before and 280 ms after.

$b = 1..1000000 | %{100; [timespan]::FromSeconds(100)}  | group

takes 20 s before and 15 s after.

It is implemented equivalently to LanguagePrimitives.Compare, but returning false where Compare would have thrown an exception.

This is adding a new public API to LanguagePrimitives, so it will probably need a committee approval.

The new APIs are:

class LanguagePrimitives {
  public static bool TryCompare(object first, object second, out int result);
  public static bool TryCompare(object first, object second, bool ignoreCase, out int result);
  /// <summary>
  /// Tries to compare first and second, converting second to the type of the first, if necessary.
  /// If a conversion is needed but fails, false is return.
  /// </summary>
  /// <param name="first">First comparison value.</param>
  /// <param name="second">Second comparison value.</param>
  /// <param name="ignoreCase">Used if both values are strings.</param>
  /// <param name="formatProvider">Used in type conversions and if both values are strings.</param>
  /// <param name="result">Less than zero if first is smaller than second, more than  zero if it is greater or zero if they are the same.</param>
  /// <returns>True if the comparison was successful, false otherwise.</returns>
  /// <exception cref="ArgumentException">The parameter <paramref name="formatProvider"/> is not a <see cref="CultureInfo"/>.</exception>
  public static bool TryCompare(
      object first, 
      object second, 
      bool ignoreCase, 
      IFormatProvider formatProvider,
      out int result);
}

The new APIs are used by ObjectCommandPropertyValue (used by sort-object and group-object) and FormatGroupManager.

PR Checklist

powercode · 2018-08-03T01:31:47Z

The remaining CodeFactor issues are intentionally left that way to match the style of similar code in the same file.

This can be used by OrderObjectBase to have a faster code path that doesn't throw exceptions.

iSazonov · 2018-08-05T11:45:51Z

This PR adds the Compare equivalent of TryConvert.

Seem typo in the PR description.

iSazonov

Please add some samples that we really get a performance win.

New public API must be approved by @powershell-committee.

iSazonov · 2018-08-05T11:57:22Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

Please add name for const argument ignoreCase: false.

iSazonov · 2018-08-05T12:07:32Z

test/powershell/engine/Api/LanguagePrimitive.Tests.ps1

We use -BeExactly for string only. Here we expect an integer and should use -Be.
Please review using -BeExactly below too.

iSazonov · 2018-08-05T12:12:13Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+                formatProvider = CultureInfo.InvariantCulture;
+            }
+
+            if (!(formatProvider is CultureInfo culture))


Why we use IFormatProvider parameter type if we want get CultureInfo?

I'm not inventing a new API here, just providing a Try alternative to the existing one.

If that API should be changed seems like a separate discussion.

iSazonov · 2018-08-05T12:23:12Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

I think we should use "dry" principle and use the function everywhere in the file.

iSazonov · 2018-08-05T12:24:24Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+            {
+                if (!(second is string secondString))
+                {
+                    if (!TryConvertTo(second, culture, out secondString))


We could replace the two if-s with one.

Compare and TryCompare.

SteveL-MSFT · 2018-08-08T22:06:26Z

@PowerShell/powershell-committee reviewed this and is fine with the api additions. Thanks.

iSazonov

LGTM with 3 minor comments.

Thanks for the great performance improvements.

iSazonov · 2018-08-09T03:54:24Z

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

            {
-                // Note that this will occur if the objects do not support
-                // IComparable.  We fall back to comparing as strings.
+                return result * (_ascendingOrder ? 1 : -1);


Do we get some win if we exclude multiplication?

I'd leave that to the compiler and the JIT. They usually do a good job on things like this.

iSazonov · 2018-08-09T03:57:01Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+
+
+
+            if (first == null && second == null)


Please remove extra empty lines above.

daxian-dbw

LGTM

PaulHigin · 2018-08-10T18:11:03Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

-                        default: return -1;
-                    }
-                }
+                return second == null ? 0 : CompareObjectToNull(second, true);


Nit: CompareObjectToNull(second, numberIsRightHandSide:true)

PaulHigin · 2018-08-10T18:11:13Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

-                    case TypeCode.Decimal: return System.Math.Sign((System.Decimal)first) < 0 ? -1 : 1;
-                    default: return 1;
-                }
+                return CompareObjectToNull(first, false);


Nit: CompareObjectToNull(first, numberIsRightHandSide:false)

PaulHigin · 2018-08-10T18:14:00Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+
+            if (first == null)
+            {
+                result = CompareObjectToNull(second, true);


Nit: CompareObjectToNull(second, numberIsRighthandSide:true)

PaulHigin · 2018-08-10T18:14:15Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+            {
+                // If it's a positive number, including 0, it's greater than null
+                // for everything else it's less than zero...
+                result = CompareObjectToNull(first, false);


Nit: Same comment

PaulHigin · 2018-08-10T18:21:30Z

src/System.Management.Automation/engine/LanguagePrimitives.cs

+            {
+                if (!(second is string secondString))
+                {
+                    if (!TryConvertTo(second, culture, out secondString))


TryConvertTo() uses ConversionData.Invoke() which throws exceptions for invalid casts. Have you considered adding a "TryInvoke" method to avoid the exception, for performance?

@PaulHigin I recently changed TryConvertTo to check the result of FigureConvertion and exit early, without invoking ConversionData.Invoke() if the ConvertionRank is None.

So, yes, I have avoided the exception in many but not all cases. Interesting take to put it on ConversionData. That is arguably where it belongs.

PaulHigin · 2018-08-10T18:22:41Z

Whoops, I just saw that this has already been merged. None of my comments are blocking.

powercode requested review from BrucePay, JamesWTruher, PaulHigin, dantraMSFT and daxian-dbw as code owners August 3, 2018 00:27

powercode force-pushed the FasterCompare branch 3 times, most recently from d767118 to 296db72 Compare August 3, 2018 00:49

powercode changed the title ~~Adding LanguagePrimitives.TryCompare to provide faster comparisons~~ WIP: Adding LanguagePrimitives.TryCompare to provide faster comparisons Aug 3, 2018

powercode force-pushed the FasterCompare branch 2 times, most recently from ae34a95 to 0553541 Compare August 3, 2018 06:48

powercode added 3 commits August 3, 2018 09:00

Adding the Compare equivalent to TryConvert - TryCompare.

575c3f3

This can be used by OrderObjectBase to have a faster code path that doesn't throw exceptions.

Adding tests for LanguagePrimitives.TryConvert

6b96f16

Using LanguagePrimitives.TryCompare where we don't want exceptions.

1416a78

powercode force-pushed the FasterCompare branch from 0553541 to 1416a78 Compare August 3, 2018 07:01

powercode changed the title ~~WIP: Adding LanguagePrimitives.TryCompare to provide faster comparisons~~ Adding LanguagePrimitives.TryCompare to provide faster comparisons Aug 3, 2018

iSazonov reviewed Aug 5, 2018

View reviewed changes

iSazonov added the Review - Committee The PR/Issue needs a review from the PowerShell Committee label Aug 5, 2018

powercode added 2 commits August 6, 2018 23:38

Refactoring comparison to null into a method, and calling it from both

1e82b95

Compare and TryCompare.

Naming boolean argument ignoreCase.

cd6d97f

daxian-dbw requested review from SteveL-MSFT and removed request for JamesWTruher and PaulHigin August 6, 2018 21:49

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Aug 8, 2018

-BeExactly -> -Be

1237a65

iSazonov approved these changes Aug 9, 2018

View reviewed changes

iSazonov assigned daxian-dbw Aug 9, 2018

daxian-dbw approved these changes Aug 9, 2018

View reviewed changes

Remove extra new lines in LanguagePrimitives.cs

74effbd

daxian-dbw merged commit 3aaa181 into PowerShell:master Aug 10, 2018

PaulHigin reviewed Aug 10, 2018

View reviewed changes

powercode deleted the FasterCompare branch August 10, 2018 18:52

Conversation

powercode commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

powercode commented Aug 3, 2018

Uh oh!

iSazonov commented Aug 5, 2018

Uh oh!

iSazonov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powercode Aug 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT commented Aug 8, 2018

Uh oh!

iSazonov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PaulHigin commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

powercode commented Aug 3, 2018 •

edited

Loading

iSazonov left a comment •

edited

Loading

powercode Aug 6, 2018 •

edited

Loading

iSazonov left a comment •

edited

Loading