Delay slicing in list pattern until all other element patterns are matched #70615
Conversation
ghost
added
the
Community
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
|
Can I have a review here please? |
|
We may want to run this by LDM and/or compat council to make sure everyone is ok with this. Personally, I like this, and it fits with what we've said about patterns. But we should make sure everyone is on the same page here. @jcouv would you want to drive this? |
|
I guess, if there are arguments against the change as a whole we can at least limit this to only apply to arrays/strings since they are special cases and rely on |
|
And if this question will be considered by LDM, it would be better to broaden its scope and discuss the possibility of the compiler changing the order of evaluation in list patterns without being tied to any specific pattern type. So for instance a pattern on a string array like
This is the most efficient order of evaluation here since constant is cheaper to check than property access + greater than operator and slicing is the most expensive one which always evaluates to true. Right now compiler checks patterns in left-to-right order and this PR implements putting slicing to the end, but it would be better if language allowed compiler to be as flexible as possible to deliver the best performance in every particular case |
Sure. Added the topic to LDM queue: https://github.com/dotnet/csharplang/blob/main/meetings/2023/README.md#schedule-asap |
|
Unblocked the PR as LDM approved this design (delaying the slice). #Closed |
src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder_ListPatterns.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder_ListPatterns.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder_ListPatterns.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Portable/Binder/DecisionDagBuilder_ListPatterns.cs
Outdated
Show resolved
Hide resolved
| IL_003a: ret | ||
| } | ||
| """); | ||
| } |
There was a problem hiding this comment.
All the added tests use pre-existing types (arrays, string, ROS) so the change is only observable through IL inspection.
Please add some tests with custom collection types where the different properties/methods are instrumented to print something, then we can directly verify effect on order of evaluation.
See SlicePattern_Subpattern for an example (Length property logs itself). #Closed
There was a problem hiding this comment.
nit: Consider adding a test with multiple "simple slice patterns". I don't think that scenario will get past initial binding, so shouldn't reach the modified code, but it may be good to check.
There was a problem hiding this comment.
The first one is done.
I don't think that scenario will get past initial binding, so shouldn't reach the modified code, but it may be good to check
Correct, that's a binding problem. And I am 99.99% sure these cases are already tested in any meaningful way possible somewhere in the binding tests. I think adding them in this PR will just create unnecessary duplicates and bloat the size of code to review. So left this one untouched. If you really feal strong, that I am wrong and such tests are actually needed here, LMK
There was a problem hiding this comment.
@jcouv I need your final word here
src/Compilers/CSharp/Test/Emit2/Semantics/PatternMatchingTests_ListPatterns.cs
Show resolved
Hide resolved
| // We faced a slice pattern with a declaration pattern of the same narrowed type inside, e.g. `[<pattern>, ..var between, <pattern>]` | ||
| // This pattern always evaluates to true, but causes slicing operation and even may allocate (e.g. when slicing an array or string) | ||
| // Thus it makes sense to hold it and "evaluate" after all other patterns in the list matched | ||
| if (slicePattern.Kind == BoundKind.DeclarationPattern && slicePattern.InputType.Equals(slicePattern.NarrowedType, TypeCompareKind.AllNullableIgnoreOptions)) |
There was a problem hiding this comment.
Is this check really necessary? the narrowed type for the slice won't escape from within the element itself. Then you can inline the local function outside the loop.
There was a problem hiding this comment.
I don't fully understand this one, can you elaborate? The narrowed type check is needed to prevent order of evaluation change when types are different since in such case we cannot guarantee that the pattern always evaluates to true (see #69053 for instance) and thus we cannot tell, whether it is cheaper to perform slicing pattern check or check of the last N patterns in the list
There was a problem hiding this comment.
The narrowed type check is needed to prevent order of evaluation change when types are different since in such case we cannot guarantee that the pattern always evaluates to
true
Matching the slice pattern last will change the ordering (of the Slice call) anyways? but that is obviously not a requirement - allowing this optimization. I don't think narrowed type of the slice pattern is at issue here because it won't affect any other patterns e.g. the remaining subpatterns in the list.
we cannot tell, whether it is cheaper to perform slicing pattern check or check of the last N patterns in the list
As you mentioned, Slice may actually allocate for non-span types so it's worth considering making it the last test to perform regardless of what subpattern we have. As for overall ordering of the tests based on cost, there's #29033.
Unless this complicates subsumption checking I think it should be ok to do this uniformly. I may have missed something from previous discussions, but @jcouv can weigh in on whether or not that's an appropriate change to make.
There was a problem hiding this comment.
I'd be okay to generalize the optimization.
|
@jcouv I've finally returned to this PR, would like a look from you) |
| """; | ||
|
|
||
| var comp = CreateCompilationWithIndexAndRangeAndSpan([source, TestSources.GetSubArray]); | ||
| comp.VerifyEmitDiagnostics(); |
There was a problem hiding this comment.
For tests that don't have errors, it would be good to verify execution.
I'd even recommend switching some of those tests away from arrays or ReadOnlySpans to a custom countable/indexable/sliceable type so that the order of evaluation can be made apparent. Verifying that the output is something like index[0] index[^1] slice will more effectively capture the intent and avoid unintentional regressions than a block of IL. (like SlicePattern_LastDeclarationPatternOfSameType_CustomTypeWithSideEffects you added below)
There was a problem hiding this comment.
For tests that don't have errors, it would be good to verify execution.
Good idea to verify that behavior stays the same for most cases, working on that
I'd even recommend switching some of those tests away from arrays or ReadOnlySpans to a custom countable/indexable/sliceable type so that the order of evaluation can be made apparent
There are 3 groups of tests added in this PR: when slice is in between other patterns, when it is the first and when it is the last. In each group there are 4 tests: 2 tests arrays with the same type and a narrowed type, test of a string, test of ROS and a test with custom side-effecting type. Each test has its own purpose: arrays and strings have special slicing behavior, so it is good to verify them separately, ROS represents a case when slicing is completely pure and test with side effect well... tests behavior change when there is a side effect (remember that this should represent a minority of real-world cases, so we shouldn't focus on behavior changes too much). Thus I wouldn't switch existing tests from one type to another, rather add additional tests on top of what we already have.
Verifying that the output is something like
index[0] index[^1] slice
I didn't understand this one. Can you elaborate please? We already cover cases with side effects, what other types of side effects can we check for? Maybe we can modify existing side effect tests to verify more things at once? Can you please provide a prototype of a test you want to see
| """; | ||
|
|
||
| var comp = CreateCompilationWithIndexAndRangeAndSpan([source, TestSources.GetSubArray]); | ||
| comp.VerifyEmitDiagnostics(); |
There was a problem hiding this comment.
This would be a good test to show side-effect. It's much harder to figure out from the IL block.
There was a problem hiding this comment.
Can you clarify what do you mean by "side-effect" here? We usually reffer to side effects as something, which is not a part of the main code flow or a potential unintentional behavior from user's perspective. The review is pointing to a test of an array, which by design should fully keep its original behavior with the only observable change being performance improvement in certain scenarios. So I wouldn't say there is a side effect of the optimization when dealing with arrays. Did you mean just to verify runtime behavior (which I'm gonna do anyway) or what?
| // We faced a slice pattern with a declaration pattern of the same narrowed type inside, e.g. `[<pattern>, ..var between, <pattern>]` | ||
| // This pattern always evaluates to true, but causes slicing operation and even may allocate (e.g. when slicing an array or string) | ||
| // Thus it makes sense to hold it and "evaluate" after all other patterns in the list matched | ||
| if (slicePattern.Kind == BoundKind.DeclarationPattern && slicePattern.InputType.Equals(slicePattern.NarrowedType, TypeCompareKind.AllNullableIgnoreOptions)) |
There was a problem hiding this comment.
Need to resolve #70615 (comment) before working on this one
|
@dotnet/roslyn-compiler for another review. Thanks |
|
@dotnet/roslyn-compiler for second review. Thanks |
Fixes: #60091