[SourceKit] Support cancellation of requests while an AST is being built #38782

ahoppen · 2021-08-06T16:08:55Z

This commit refactors the way ASTs are being built in SourceKit and how SwiftASTConsumers are served by the built ASTs. SwiftASTManager.h should give an overview of the new design.

This commit does not change the cancellation paradigm in SourceKit (yet). That is, subsequent requests with the same OncePerASTToken still cancel previous requests with the same token. But while previously, we were only able to cancel requests that haven’t started an AST build yet, we can now also cancel the AST build of the to-be-cancelled requests.

With this change in place, we can start looking into explicit cancellation of requests or other cancellation paradigms.

rdar://83391478

ahoppen · 2021-08-09T14:57:25Z

@swift-ci Please build toolchain macOS

swift-ci · 2021-08-09T20:44:09Z

macOS Toolchain
Download Toolchain
Git Sha - c599eaf235a41fdebbd3c551e12affce57d065e5

Install command
tar -zxf swift-PR-38782-1068-osx.tar.gz --directory ~/

ahoppen · 2021-08-10T12:48:26Z

@swift-ci Please smoke test

benlangmuir · 2021-08-10T17:20:02Z

include/swift/Basic/LangOptions.h

+    /// typechecking should be aborted at the next possible opportunity.
+    /// This is used by SourceKit to cancel requests for which the result is no
+    /// longer of interest.
+    std::shared_ptr<bool> CancellationFlag = nullptr;


@xedin does this seem okay to put the shared cancellation state in TypeCheckerOptions?

@ahoppen this should wrap a std::atomic<bool> to avoid data races, and use load(std::memory_order_relaxed) and store(true, std::memory_order_relaxed).

I thought about race conditions here a bit and think it should be fine with a plain pointer. We should only have one location setting the cancellation flag to true and that’s independent of what it’s previous content was, so there shouldn’t be any write races. And if setting the flag to true doesn’t immediately propagate to the thread that is building the AST, it’s also fine since nobody expects the cancellation to be immediate – it’s totally fine for the type checker to e.g. continue type checking for a few more cycles. But I might be missing something.

But we could of course make it a std::atomic. Not quite sure if that will add some performance overhead though…

We cannot ignore the read-write race:

The compiler is free optimize this code based on the assumption no other thread modifies the value, since it's not valid to do so.

TSan will raise an error on this race.

But we could of course make it a std::atomic. Not quite sure if that will add some performance overhead though…

Using relaxed memory order atomic operations shouldn't cost much. On x86_64 it should be basically free.

OK, I changed it to a std::atomic.

Out of curiosity though: How would the race condition manifest at runtime except for the TSan failure?

How would the race condition manifest at runtime except for the TSan failure?

Likely it wouldn't manifest in practice under any current compiler, but in principle it could only check the flag once and never again (for a single AST build).

tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp

tools/SourceKit/lib/SwiftLang/SwiftASTManager.h

tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp

ahoppen · 2021-08-11T15:18:49Z

@swift-ci Please smoke test

ahoppen · 2021-08-12T06:47:29Z

@swift-ci Please smoke test

ahoppen · 2021-08-25T10:56:01Z

@swift-ci Please smoke test Windows

Once this flag is set, the type checker will bail out with a “expression to complex” error.

ahoppen · 2021-09-22T11:10:52Z

@swift-ci Please smoke test

benlangmuir

Basically looking good, just a few minor things.

benlangmuir · 2021-09-22T16:28:41Z

include/swift/Basic/LangOptions.h

+    /// typechecking should be aborted at the next possible opportunity.
+    /// This is used by SourceKit to cancel requests for which the result is no
+    /// longer of interest.
+    std::shared_ptr<std::atomic<bool>> CancellationFlag = nullptr;


@xedin or @hborla any concerns about this mutable state living in TypeCheckerOptions?

I don't have concerns but I've also never touched this thing 🙂 @CodaFi do you have any thoughts?

It’s a fine place to put it to start. I just hope we move off a cooperative cancellation model and push this into the request evaluator at some point.

test/SourceKit/Misc/stats.swift

tools/SourceKit/lib/SwiftLang/SwiftASTManager.cpp

unittests/SourceKit/SwiftLang/CursorInfoTest.cpp

This commit refactors the way ASTs are being built in SourceKit and how `SwiftASTConsumer`s are served by the built ASTs. `SwiftASTManager.h` should give an overview of the new design. This commit does not change the cancellation paradigm in SourceKit (yet). That is, subsequent requests with the same `OncePerASTToken` still cancel previous requests with the same token. But while previously, we were only able to cancel requests that haven’t started an AST build yet, we can now also cancel the AST build of the to-be-cancelled requests. With this change in place, we can start looking into explicit cancellation of requests or other cancellation paradigms.

ahoppen · 2021-09-23T09:54:15Z

@swift-ci Please test

benlangmuir

I'd still like someone other than me to comment on putting the cancellation state into the TypeCheckerOptions. LGTM otherwise.

Seems that VS2019 and other C++ standard libraries include <atomic> as part of either <string> or <vector>, but not VS2017. When #38782 landed a new usage of std::atomic started creating compilation problems in the VS2017 builders (eg. https://ci-external.swift.org/job/oss-swift-windows-x86_64/8736/). Including the <atomic> header solves the problem.

ahoppen force-pushed the pr/sourcekit-cancellation branch from 9fa75c3 to c599eaf Compare August 9, 2021 14:56

ahoppen changed the title ~~[WIP][SourceKit] Allow cancellation of AST building~~ [SourceKit] Support cancellation of requests while an AST is being built Aug 9, 2021

ahoppen marked this pull request as ready for review August 10, 2021 12:48

ahoppen requested review from benlangmuir and akyrtzi August 10, 2021 12:48

benlangmuir reviewed Aug 10, 2021

View reviewed changes

ahoppen force-pushed the pr/sourcekit-cancellation branch from c599eaf to 3309b78 Compare August 11, 2021 15:10

ahoppen force-pushed the pr/sourcekit-cancellation branch from 3309b78 to 082cea4 Compare August 12, 2021 06:47

[Sema] Add cancellation flag to typechecker options

998a787

Once this flag is set, the type checker will bail out with a “expression to complex” error.

ahoppen force-pushed the pr/sourcekit-cancellation branch from 082cea4 to 2694d0b Compare September 22, 2021 11:10

benlangmuir reviewed Sep 22, 2021

View reviewed changes

ahoppen force-pushed the pr/sourcekit-cancellation branch from 2694d0b to 7a80034 Compare September 23, 2021 09:54

benlangmuir approved these changes Sep 23, 2021

View reviewed changes

ahoppen merged commit 4521c99 into swiftlang:main Sep 28, 2021

ahoppen deleted the pr/sourcekit-cancellation branch September 28, 2021 18:08

drodriguez mentioned this pull request Oct 5, 2021

[windows][vs2017] Add missing <atomic> include. #39580

Merged

[SourceKit] Support cancellation of requests while an AST is being built #38782

[SourceKit] Support cancellation of requests while an AST is being built #38782

Uh oh!

Conversation

ahoppen commented Aug 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahoppen commented Aug 9, 2021

Uh oh!

swift-ci commented Aug 9, 2021

Uh oh!

ahoppen commented Aug 10, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Aug 11, 2021

Uh oh!

ahoppen commented Aug 12, 2021

Uh oh!

ahoppen commented Aug 25, 2021

Uh oh!

ahoppen commented Sep 22, 2021

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ahoppen commented Sep 23, 2021

Uh oh!

benlangmuir left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen commented Aug 6, 2021 •

edited

Loading