Allow cancellation during extract method's symbol walking #18121
Conversation
|
@amcasey, |
src/compiler/symbolWalker.ts
Outdated
| @@ -158,6 +162,10 @@ namespace ts { | |||
| } | |||
|
|
|||
| function visitSymbol(symbol: Symbol): boolean { | |||
| if (cancellationToken) { | |||
There was a problem hiding this comment.
I have 2 fairly ignorant questions
- Are cancellation checks expensive (I seem to recall that it's based on checking a named pipe, which sounds scary)? Is it throttled?
- How many times do we expect
visitSymbolto be called in a "typical" operation?
I guess the overall question I'm asking is whether you're fairly sure this check isn't a performance burden.
There was a problem hiding this comment.
FYI: Some crude overhead measurements from back when I prototyped this: https://github.com/billti/cancellation#overhead . Definitely use the throttling mechanism. We've seen significant perf regressions when this wasn't done in prior cancellation checks.
There was a problem hiding this comment.
As Vlad said, cancellation checks are throttled. Having said that, I don't have a sense of how expensive cancellability is.
| @@ -932,7 +933,8 @@ namespace ts.refactor.extractMethod { | |||
| scopes: Scope[], | |||
| enclosingTextRange: TextRange, | |||
| sourceFile: SourceFile, | |||
| checker: TypeChecker) { | |||
| checker: TypeChecker, | |||
| cancellationToken: CancellationToken) { | |||
There was a problem hiding this comment.
With the other changes, I don't think this argument is used anywhere anymore.
There was a problem hiding this comment.
If you mean cancellationToken, I believe it is closed over in the accept lambda below.
There was a problem hiding this comment.
@amcasey Ack, sorry, thought I was reading a different file here. Nevermind. All good.
|
Per @weswigham's suggestion, handle cancellation in the Accept predicate, rather than adding a new SymbolWalker API. |
| @@ -932,7 +933,8 @@ namespace ts.refactor.extractMethod { | |||
| scopes: Scope[], | |||
| enclosingTextRange: TextRange, | |||
| sourceFile: SourceFile, | |||
| checker: TypeChecker) { | |||
| checker: TypeChecker, | |||
| cancellationToken: CancellationToken) { | |||
There was a problem hiding this comment.
@amcasey Ack, sorry, thought I was reading a different file here. Nevermind. All good.
amcasey
changed the title
Allow cancellation during extract method's symbol walking
...since it's potentially expensive.