Prevent freezes in AspectSyncSourceToTargetMap.getSourceToTargetMap #8029
Conversation
github-actions
bot
added
product: CLion
ujohnny
force-pushed
the
enovozhilov/prevent-freeze
branch
4 times, most recently
from
39c6036 to
de678b5
Compare
getSourceToTargetMap might be called under read action and it's a quite heavy operation. we can bring cancellation checks into it, but a. it might be called under non-cancellable read action b. it might take tens of seconds to compute, so no readaction can survive this long so if we're called under readaction, we just return null and schedule background computation, and hope we will be called some time later. but we expect to be called from ProjectTargetManagerImpl on the background thread, so we can build map, but in case we're invoked from someone else before, background computation will prevent the freeze
ujohnny
force-pushed
the
enovozhilov/prevent-freeze
branch
from
de678b5 to
89506c6
Compare
LeFrosch
left a comment
LeFrosch
left a comment
There was a problem hiding this comment.
Overall looks good just a bunch of nit picks. And I changed my mind, this is probably more reliable then my Deferred idea.
| scope.launch { | ||
| get(key, computable) | ||
| } |
There was a problem hiding this comment.
Will this swallow exceptions?
| @@ -0,0 +1,55 @@ | |||
| package com.google.idea.blaze.base.sync | |||
| sealed class TryLockResult<out T> { | ||
| data class Acquired<T>(val value: T) : TryLockResult<T>() | ||
| data object NotAcquired : TryLockResult<Nothing>() | ||
| } |
There was a problem hiding this comment.
Isn't this basically an Optional?
| /** Computes a cache on the project data. */ | ||
| @Service(Service.Level.PROJECT) | ||
| class SyncCache(private val project: Project, private val scope: CoroutineScope) { | ||
| /** Computes a value based on the sync project data. */ |
There was a problem hiding this comment.
Wrong indentation of 4 spaces instead of 2
There was a problem hiding this comment.
interesting as I use https://github.com/google/styleguide/blob/gh-pages/intellij-java-google-style.xml, but it does not have kotlin settings, moreover there's no kotlin styleguide in this repo at all
| fun compute(project: Project, projectData: BlazeProjectData): T? | ||
| } | ||
|
|
||
| private data class Data(val cache: MutableMap<Any, Any?>) |
There was a problem hiding this comment.
What's the benefit of wrapping the Map in the data class?
| * Encapsulates the data into [hidden] and allows retriable access to it | ||
| * with cancellation checks | ||
| */ | ||
| class LockCriticalSection<Hidden : Any>(private val hidden: Hidden) { |
There was a problem hiding this comment.
Since we only use this class for the SyncCache and it's only ever instantiated for a HashMap, we could drop some of the generic parameters in favor of specialization?
4 participants
getSourceToTargetMap might be called under read action and it's
a quite heavy operation. we can bring cancellation checks into it,
but
a. it might be called under non-cancellable read action
b. it might take tens of seconds to compute, so no readaction
can survive this long
so if we're called under readaction, we just return null and schedule
background computation, and hope we will be called some time later