Comparable helper to compare by components by ivanbasov · Pull Request #37894 · dotnet/roslyn

ivanbasov · 2019-08-10T01:25:04Z

A helper to compare objects by a series of components. If the first component are different, return the comparison. Otherwise, proceed with the second component and so on.

genlu · 2019-08-12T18:35:06Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/FilterResult.cs

-            }
-
-            return this.CompletionItem.IsPreferredItem().CompareTo(other.CompletionItem.IsPreferredItem());
+            return IComparableHelper.CompareTo(


I'm a little concerned about the extra allocations here. Could this somehow end up in some hot path during completion and cause perf regression? #Resolved

yeah, agreed. i don't see how this wouldn't be a problem. #Resolved

CyrusNajmabadi · 2019-08-12T18:54:18Z

src/Compilers/Core/Portable/InternalUtilities/IComparableHelper.cs

+{
+    internal static class IComparableHelper
+    {
+        public static int CompareTo<T>(T first, T second, params Func<T, IComparable>[] comparingComponents)


not certin we want this. will this be double allocation (the function, and the array?). Or do you intend to cache that all callside.

CyrusNajmabadi · 2019-08-12T18:55:29Z

src/Compilers/Core/Portable/InternalUtilities/IComparableHelper.cs

+    internal static class IComparableHelper
+    {
+        public static int CompareTo<T>(T first, T second, params Func<T, IComparable>[] comparingComponents)
+        {


why add to compiler layer? just have in IDE layer.

CyrusNajmabadi · 2019-08-12T18:55:45Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/FilterResult.cs

+                    ? f.CompletionItem.Rules.MatchPriority
+                    : MatchPriority.Default,
+                f => f.CompletionItem.FilterText.GetCaseSensitivePrefixLength(f.FilterText),
+                f => f.CompletionItem.IsPreferredItem());


this is a cool concept. However, we should only do this allocation once.

ivanbasov · 2019-08-16T23:39:30Z

Thank you, @genlu and @CyrusNajmabadi !
I have addressed the feedback. Please re-review. #Resolved

ivanbasov · 2019-08-20T18:21:16Z

@genlu and @CyrusNajmabadi please review

#Resolved

CyrusNajmabadi

Seems fine :) Though i would prefer to see this used in more places to justify the code change. other examples might be: CodeFixService.GetFixesAsync, VirtualTreePoint.CompareTo, AddImport.Reference.CompareTo, CompletionHelper.CompareItems, MethodExtractor.Compare, WrapItemsAction.SortActions, MemberDeclarationsOrganizer.Comparer, ModifiersOranizer.Comparer, CompareNameParts, NameSyntaxComparer, TokenComparer.

If we can really show this being used in a lot of places, hte need for a helper becomes more apparent.

Comment
Submit general feedback without explicit approval.

ivanbasov · 2019-08-21T22:26:44Z

@CyrusNajmabadi @genlu

I have applied the approach to multiple scenarios.
Here are scenario I could not apply it:

CompletionHelper.CompareItems
MemberDeclarationsOrganizer.Comparer
MemberDeclarationsOrganizer.VB
NameSyntaxComparer
NameSyntaxComparer.VB
TokenComparer
TokenComparer.VB

#Resolved

CyrusNajmabadi · 2019-08-21T22:57:05Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/FilterResult.cs

+                : MatchPriority.Default;
+
+            yield return completionItem.FilterText.GetCaseSensitivePrefixLength(filterResult.FilterText);
+            yield return completionItem.IsPreferredItem();


definitely concerned. i think allocations in these paths are not ok. I think you should have to overloads. One that takes just an ImmutableArray<IComparable>s. Another that takes an ImmutableArray<IComparableWithState> and a state arg. That way this can be allocated once, and you can call the helper from here passing along filterResult as an arg that can feed through.

Does that make sense? #Resolved

Using immutable arrays, we exclude algorithms with variable length for comparison paths like https://github.com/dotnet/roslyn/pull/37894/files#diff-322fd889d857cd92d680c3f841a26094R43 #Resolved

What is IComparableWithState? #Resolved

somehting you would have to define, but it would allow you to avoid captures by effectively stating that the delegate can be passed extra data. that way you can statically create the array of delegates, but pass data it needs dynamically. #Resolved

Thank you, @CyrusNajmabadi ! Hopefully implemented ComparerWithState.

@CyrusNajmabadi and @genlu please take a look. #Resolved

src/Workspaces/Core/Portable/Utilities/IComplarableHelper.cs

CyrusNajmabadi · 2019-08-23T02:41:37Z

src/Workspaces/Core/Portable/PatternMatching/PatternMatch.cs

+                (p, b) => !b && !p.IsCaseSensitive,
+                // Consider a match to be better if it was successful without stripping punctuation
+                // versus a match that had to strip punctuation to succeed.
+                (p, b) => p._punctuationStripped);


fwiw, i'm reviewing this but not actually confirming the lambdas match the original code. i hope you've done that. #Resolved

CyrusNajmabadi · 2019-08-23T02:42:08Z

src/Workspaces/Core/Portable/Shared/Extensions/INamespaceOrTypeSymbolExtensions.cs

-            IReadOnlyList<string> names1, IReadOnlyList<string> names2,
-            bool placeSystemNamespaceFirst)
+           IReadOnlyList<string> names1, IReadOnlyList<string> names2,
+           bool placeSystemNamespaceFirst)


undo? #Resolved

CyrusNajmabadi · 2019-08-23T02:44:09Z

src/Workspaces/Core/Portable/Utilities/ComparerWithState.cs

+
+namespace Roslyn.Utilities
+{
+    internal delegate int ComparisonWithState<in T, in S>(T x, T y, S state);


this type makes sense to me. #Resolved

CyrusNajmabadi · 2019-08-23T02:44:46Z

src/Workspaces/Core/Portable/Utilities/ComparerWithState.cs

+    /// </summary>
+    /// <typeparam name="T">Comparing type</typeparam>
+    /// <typeparam name="S">State</typeparam>
+    internal class ComparerWithState<T, S>


not certain if these classes are needed? can't you make one bit ComparisonWithState out of N individual ones? #Resolved

CyrusNajmabadi · 2019-08-23T02:46:40Z

src/Workspaces/Core/Portable/Utilities/ComparerWithState.cs

+            => new ComparerWithState<T>((t1, t2, state) => comparingMethod(t1).CompareTo(comparingMethod(t2)));
+    }
+
+    internal class ComparerWithState


static class.

i can understand these helpers to build up a final comparer-thingy out of the individual pieces. I don't quite get the need the for the above classes. #Resolved

genlu · 2019-08-27T21:08:06Z

src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/FilterResult.cs

-        }
+            => ComparerWithState.CompareTo(this, other, s_comparers);
+
+        private readonly static ImmutableArray<Func<FilterResult, IComparable>> s_comparers =


Could we keep the original comments? #Resolved

genlu

ivanbasov added Area-IDE Area-Compilers Concept-Continuous Improvement labels Aug 10, 2019

ivanbasov added this to the 16.4 milestone Aug 10, 2019

ivanbasov requested review from a team as code owners August 10, 2019 01:25

ivanbasov mentioned this pull request Aug 10, 2019

Comparable helper to compare by components #37893

Closed

genlu reviewed Aug 12, 2019

View reviewed changes

CyrusNajmabadi reviewed Aug 12, 2019

View reviewed changes

ivanbasov requested review from a team and removed request for a team August 16, 2019 23:38

ivanbasov removed the Area-Compilers label Aug 16, 2019

CyrusNajmabadi reviewed Aug 20, 2019

View reviewed changes

ivanbasov added 3 commits August 21, 2019 15:22

Comparable helper to compare by components

9c9aef7

feedback addressed

a733762

Changed to IEnumerable. More cases to use the pattern

58a5747

ivanbasov force-pushed the comparable branch from c63a109 to 58a5747 Compare August 21, 2019 22:24

CyrusNajmabadi reviewed Aug 21, 2019

View reviewed changes

src/Workspaces/Core/Portable/Utilities/IComplarableHelper.cs Outdated Show resolved Hide resolved

code review feedback

a0d8033

CyrusNajmabadi reviewed Aug 23, 2019

View reviewed changes

feedback addressed

028ba93

genlu reviewed Aug 27, 2019

View reviewed changes

genlu approved these changes Aug 27, 2019

View reviewed changes

feedback addressed

8cedcc7

ivanbasov merged commit 8d78f17 into dotnet:master Aug 28, 2019

ivanbasov deleted the comparable branch August 28, 2019 00:21

genlu mentioned this pull request Sep 26, 2019

Fix an accidental reverse caused by a bad merge #38887

Merged

CyrusNajmabadi mentioned this pull request Apr 13, 2020

Fix sorting of 'wrapping' items. #43317

Merged

Conversation

ivanbasov commented Aug 10, 2019

Uh oh!

genlu Aug 12, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 12, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

ivanbasov commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ivanbasov commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

Uh oh!

ivanbasov commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CyrusNajmabadi Aug 21, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Aug 22, 2019 • 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!

ivanbasov Aug 22, 2019 • 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!

CyrusNajmabadi Aug 22, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivanbasov Aug 23, 2019 • 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!

Uh oh!

CyrusNajmabadi Aug 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CyrusNajmabadi Aug 23, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu Aug 27, 2019 • edited by ivanbasov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

genlu left a comment

Choose a reason for hiding this comment

Uh oh!

genlu Aug 12, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Aug 12, 2019 •

edited by ivanbasov

Loading

ivanbasov commented Aug 16, 2019 •

edited

Loading

ivanbasov commented Aug 20, 2019 •

edited

Loading

ivanbasov commented Aug 21, 2019 •

edited

Loading

CyrusNajmabadi Aug 21, 2019 •

edited by ivanbasov

Loading

ivanbasov Aug 22, 2019 •

edited

Loading

ivanbasov Aug 22, 2019 •

edited

Loading

CyrusNajmabadi Aug 22, 2019 •

edited by ivanbasov

Loading

ivanbasov Aug 23, 2019 •

edited

Loading

CyrusNajmabadi Aug 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Aug 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Aug 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Aug 23, 2019 •

edited by ivanbasov

Loading

CyrusNajmabadi Aug 23, 2019 •

edited by ivanbasov

Loading

genlu Aug 27, 2019 •

edited by ivanbasov

Loading