Generate DkmEvaluationResults asynchronously #986
Conversation
|
@KevinH-MS, @amcasey, @pnelsonmsft |
| { | ||
| internal readonly DkmWorkList InnerWorkList; | ||
| private readonly CompletionRoutine<Exception> _onException; | ||
| private WorkListCompletionRoutine _completionRoutine; |
There was a problem hiding this comment.
As far as I can tell, there are three kinds of completion routines floating around, WorkListCompletionRoutine, CompletionRoutine, and DkmCompletionRoutine. However, we seem to call locals of all three types "completionRoutine". I think the code would be much easier to follow if we gave them distinct names.
There was a problem hiding this comment.
I've removed WorkListCompletionRoutine.
|
Consider splitting CompletionRoutine into |
| () => ContinueWithExceptionHandling( | ||
| var wl = new WorkList(workList, e => completionRoutine(DkmEvaluationEnumAsyncResult.CreateErrorResult(e))); | ||
| GetEvaluationResultsAndContinue(rows, results, 0, numRows, wl, inspectionContext, value.StackFrame, | ||
| () => wl.ContinueWith( |
There was a problem hiding this comment.
It looks like we could call wl.ContinueWith outside the lambda (e.g. immediately after construction).
There was a problem hiding this comment.
The lambda passed to wl.ContinueWith must be the last continuation, after any added while executing GetEvaluationResultsAndContinue.
| @@ -35,6 +35,11 @@ internal void AddWork(Action item) | |||
| _workList.Enqueue(item); | |||
| } | |||
|
|
|||
| internal int Length | |||
There was a problem hiding this comment.
If this isn't part of the real type, can we add a doc comment?
There was a problem hiding this comment.
Consider using => body for trivial properties.
|
LGTM |
Generate DkmEvaluationResults asynchronously Generate subsequent DkmEvaluationResults asynchronously rather than in the completion from previous DkmEvaluationResults to avoid stack overflow when many items are requested and evaluation is not already asynchronous (when EvaluateDebuggerDisplayString is not used).
| () => | ||
| { | ||
| results[index] = result; | ||
| if (index < numRows - 1) |
There was a problem hiding this comment.
Should this be "index < numRows"?
There was a problem hiding this comment.
numRows - 1 since we're deciding whether to evaluate row index + 1.
There was a problem hiding this comment.
Would we get the same result if we changed it to "index < numRows" and removed the "else..."? That is, would the outer if catch it and call the completion routine when we call GetEvaluationResultsAndContinue with "index + 1"?
There was a problem hiding this comment.
Actually, could we remove the if/else from the lambda entirely and just call GetEvaluationResultsAndContinue (relying on the outer if to call the completionRoutine)?
Generate subsequent DkmEvaluationResults asynchronously rather than in completion from previous DkmEvaluationResults to avoid stack overflow when many items are requested and evaluation is not already asynchronous (when EvaluateDebuggerDisplayString is not used).