[SourceKit] Add a request kind to notify dependencies are updated #35768
Conversation
|
@swift-ci Please smoke test |
rintaro
force-pushed
the
sourcekit-dependencyupdated
branch
from
6762213 to
a4eee0e
Compare
|
@swift-ci Please smoke test |
lib/IDE/CompletionInstance.cpp
Outdated
| @@ -586,6 +598,11 @@ bool CompletionInstance::shouldCheckDependencies() const { | |||
| return threshold <= now; | |||
| } | |||
|
|
|||
| void CompletionInstance::invalidateCachedCompilerInstance() { | |||
| std::lock_guard<std::mutex> lock(mtx); | |||
| CachedCIInvalidated = true; | |||
There was a problem hiding this comment.
It looks to me that you have shared access of CachedCIInvalidated but without always using the mutex.
I think you can just make it a std::atomic<bool>.
There was a problem hiding this comment.
My bad, I moved
// Clear the cached CI if it's invalidated.
if (CachedCIInvalidated) {
clearCachedCompilerInstance();
}
inside the mutex. Do you still think it should be std::atomic<bool>?
There was a problem hiding this comment.
I don't feel safe CachedCIInvalidated is mutated during the performOperation() anyways. Using mutex to guard it is still needed.
There was a problem hiding this comment.
Is the modification in CompletionInstance::cacheCompilerInstance() protected?
I would suggest it's better to use std::atomic<bool> because:
- One less thing to worry about accessing out of a mutex. With later changes you may end up either not protecting access to this field or blocking the modification of the new request on the mutex longer than you'd expect.
- It doesn't matter when this is getting modified or not, what matters is when you choose to check for it. It is like a "hint", e.g. say you had a "cancel" boolean that can be set while operation is running and then you check it at certain points, what matters is just to make it's use thread-safe.
How about renaming it to CachedCIShouldBeInvalidated, then it is more clear that modifying this doesn't have an effect until we check it at certain points.
There was a problem hiding this comment.
Yes cacheCompilerInstance() is private method and only called inside performOperation() after std::lock_guard<std::mutex> lock(mtx).
Anyways, I agreed. Updated to use std::atomic<bool>
rintaro
force-pushed
the
sourcekit-dependencyupdated
branch
from
a4eee0e to
946f9c6
Compare
|
@swift-ci Please smoke test |
rintaro
force-pushed
the
sourcekit-dependencyupdated
branch
2 times, most recently
from
e02d8ed to
d048342
Compare
| @@ -49,9 +49,11 @@ class CompletionInstance { | |||
| llvm::sys::TimePoint<> DependencyCheckedTimestamp; | |||
| llvm::StringMap<llvm::hash_code> InMemoryDependencyHash; | |||
| unsigned CachedReuseCount = 0; | |||
| std::atomic<bool> CachedCIShouldInvalidated; | |||
There was a problem hiding this comment.
Suggesting CachedCIShouldInvalidated -> CachedCIShouldBeInvalidated
|
|
||
| // Mark the cached compiler instance "invalidated". It will be cleared in | ||
| // next completion. (Thread safe.) | ||
| void invalidateCachedCompilerInstance(); |
There was a problem hiding this comment.
Suggesting invalidateCachedCompilerInstance -> markCachedCompilerInstanceShouldBeInvalidated
lib/IDE/CompletionInstance.cpp
Outdated
| @@ -586,6 +597,11 @@ bool CompletionInstance::shouldCheckDependencies() const { | |||
| return threshold <= now; | |||
| } | |||
|
|
|||
| void CompletionInstance::invalidateCachedCompilerInstance() { | |||
| std::lock_guard<std::mutex> lock(mtx); | |||
There was a problem hiding this comment.
The lock guard is not necessary right?
lib/IDE/CompletionInstance.cpp
Outdated
| @@ -618,6 +634,9 @@ bool swift::ide::CompletionInstance::performOperation( | |||
| // the cached completion instance. | |||
| std::lock_guard<std::mutex> lock(mtx); | |||
|
|
|||
| // Clear the cached CI if it's invalidated. | |||
| clearCachedCompilerInstanceIfNeeded(); | |||
There was a problem hiding this comment.
Why was it necessary to introduce this extra function and not just add an additional check inside performCachedOperationIfPossible()?
rintaro
force-pushed
the
sourcekit-dependencyupdated
branch
from
d048342 to
366965c
Compare
|
How does this look @akyrtzi ? |
rintaro
force-pushed
the
sourcekit-dependencyupdated
branch
from
366965c to
27dc2cf
Compare
|
@swift-ci Please smoke test |
At this point, SourceKit clears the cached compiler instance for code completion if exist.