Internal Query: Fixes ToList work #2060
Conversation
bchong95
requested review from
j82w,
kirankumarkolli and
sboshra
as code owners
| @@ -83,18 +83,26 @@ public async ValueTask<bool> MoveNextAsync(ITrace trace) | |||
| else | |||
| { | |||
| // left most and any non null continuations | |||
| List<FeedRangeState<QueryState>> feedRangeStates = crossPartitionState | |||
| IOrderedEnumerable<FeedRangeState<QueryState>> feedRangeStates = crossPartitionState | |||
There was a problem hiding this comment.
Please add tests to prevent future regressions.
There was a problem hiding this comment.
We don't even have a reproduce, so there is not test. It also never affected correctness, so what kind of test are you looking for?
There was a problem hiding this comment.
How do we know the issue is fixed then? Can't we have a test that defines the expected behavior and verifies this change is actually fixing the issue?
There was a problem hiding this comment.
Or at least have a test that defines the behavior that we had before the code change to assert that the new code does not regress/change it?
There was a problem hiding this comment.
We know that the problem container only had 2 physical partitions. It should be easy to unit test all the possible combinations with 2 items [(a,b)(b,a)(null,a)(null,null)(b,null)(a,a)], and verify they all complete in reasonable time.
There was a problem hiding this comment.
I enumerated the combinations. Nothing reproduced. Send a reproduce and create a work item. This PR is just to take query code out of the stack. If the high CPU is caused by doing string comparison, then we know to route to dotnet team.
There was a problem hiding this comment.
If the diagnostics does not include the necessary information to reproduce the issue please update it to include it. Then capture the information to reproduce the issue.
There was a problem hiding this comment.
The user didn't even have diagnostics for the reported issue. Im sure tracing would show time being spent in move next async if that were the case.
| List<ParallelContinuationToken> activeParallelContinuationTokens = new List<ParallelContinuationToken>(); | ||
| for (int i = 0; i < feedRangeStates.Count; i++) | ||
| { | ||
| FeedRangeState<QueryState> firstState = feedRangeStates.First(); |
There was a problem hiding this comment.
Remove the internal from the PR title. This can impact external customers.
Query: Fixes high CPU caused by LINQ orderBy logic
There was a problem hiding this comment.
We don't even know if it fixes anything. No reproduce means no validation.
There was a problem hiding this comment.
We shouldn't make a change unless there is a reason. How do you know this will not make it worse?
There was a problem hiding this comment.
It's removing instructions from the compare operator thus faster.
There was a problem hiding this comment.
There is no benchmark so how do you know it is faster?
Internal Query: Fixes ToList work
Description
Internal customer ran into issue where ToList() was consuming too many CPU cycles. We don't know why this is the case, but we can atleast avoid the allocation for it and remove a comparer this is just a glorified wrapper around string comparer.