VOLUNTEERS REQUESTED: Eliminate (or at least minimize) "Slow operations are prohibited on EDT"

[Scott Wells]

JetBrains has been making a hard push in recent versions to make/keep the IDE performance snappy. In a Swing application, a critical aspect of that is ensuring that anything that happens on the EDT (Event Dispatch Thread) happens very quickly. They’ve started helping plugin devs know about calls to potentially slow plugin SDK operations when invoked on the EDT, manifesting as stack traces in idea.log with the message “Slow operations are prohibited on EDT”.

IC2 produces quite a few of these due to needing to query information about the project/module structure frequently, primarily to discern the configuration, e.g., the location and contents of the sfdx-project.json file and the IC2-specific module config. While these queries do not seem to have any negative impact on performance, they do result in very noisy log file contents when trying to troubleshoot legitimate issues.

I’m currently working to minimize (ideally eliminate) these calls from the EDT and have local changes that seem to address the vast majority of them. However, as you can imagine, such changes are non-trivial and require significant testing beyond just normal automated (or even manual) functional/regression testing.

I’m planning to keep a test version of the current released IC2 build available here for any brave souls who would like to help “burn in” these changes. For those who volunteer to assist, I’d be asking for these builds to be installed instead of the official builds posted to the JetBrains plugin repository and, if issues are encountered that aren’t present on the official build, to report them here so that they can be addressed. Of course, if those issues have high impact, users can (and should) return to the official build (ideally once the issues have been reported).

Volunteers should add themselves as watchers of this issue tracker entry so that 1) I have an idea of how many users are on these builds; 2) those using these builds can know when an updated build is available and move to that to continue providing feedback on the latest-and-greatest.

[Scott Wells]

If you'd like to help test these changes, please:

1. Add yourself as a watcher of this issue so I know how many folks are trying it and so that you're notified of updates.
2. Install the attached build by downloading (but not extracting) it and using Settings | Plugins | Install plugin from disk (under the gear drop-down menu).
3. Then pretty much just use the product as normal.

If you see issues unique to this build -- functional or performance -- please report them here as comments. If the issues are impeding your work, please just uninstall the test build and install the official build, but please do report those issues so that I can address them.

Thanks for the help!

[Adam Stepanek]

What exactly we should look for? Any performance differences?

[Scott Wells]

I wouldn’t expect there to be much of a performance difference, but certainly if you find anything to be slower, I’d like to know about it. The big thing I’d ask you (and others) to look for would be anything that isn’t working properly on the test build vs. the official build. Again, I wouldn’t expect that to be the case, but if something is behaving differently/incorrectly, please let me know, ideally with expected vs. actual behavior and clear steps to reproduce.

[Scott Wells]

For those helping test these changes, here's a new build based on 2.3.1.3. Please update to it if you wish to continue helping test.

Also, if you've been using the previous build, have you seen any issues or changes in expected behavior?

Thanks again!

[Arsenii Skvortsov]

I only recalled I was participating in this beta testing after seeing your message. ☺ I’ve being using Deploy (no tests), Run Tests (+Coverage), SOQL, Anonymous Apex, Log Analyzer - didn’t notice any issue.

[Scott Wells]

That’s great to hear, @Arsenii Titorenko . I didn’t know if this was more of a “silence is golden” type of thing or it just wasn’t getting exercised by others. I’ll give it a few more weeks of burn-in – I’m out of pocket all of next week anyway, so the timing is good – and if things still look solid after that, I’ll plan to roll it into the official releases.

I’d love to hear from any others who have been using this build as a daily-driver as well.

[Scott Wells]

Another build to align with 2.3.1.4 which is a very small hotfix update.

[Tony White]

Not sure if Webstorm has hung or not, but after doing a metadata retrieval, and it is ‘Preparing retrieved files’,
I do not seem to be able to click into the IDE and continue editing files…

[Tony White]

Ended up having to force quit, and then updated to the latest and it hung again doing the retrieve, have emailed you a copy of the log.

[Tony White]

IF I want to go back to the standard version aka non beta version whats the best way I can do that?

Do I need to uninstall and reinstall from marketplace? will I lose settings etc?

[Scott Wells]

Yes just uninstall the test build and install the official build. You will not lose any settings. The project will likely need to reindex, but that should be it.

[Tony White]

FYI - After reverting back to the non-beta version, I no longer get hangs on metadata retrieval

[Scott Wells]

That’s certainly damning evidence. I’ll be back at my normal desk starting Monday and will dig into the provided thread dumps. Hopefully they’ll clearly identify what’s causing that deadlock/freeze. Once I address it, I’ll post a new build and let folks know here.

[Scott Wells]

Okay, hopefuly this build should resolve the deadlock that Tony reported. It was actually due to one of the remaining usage of a non-EDT-safe API that was quite simple to eliminate (at least once I found the proper alternative).

Can those on the test build please update to this build? And can Tony specifically install it and verify that it resolves the deadlock (or, if it doesn't, that's good to know as well)?

Thanks!

[Tony White]

Updated the plugin and tried to do a retrieve and it has hung again… will email logs and thread dump

[Scott Wells]

Thanks for checking. Basically a similar type of thing going on where it’s ending up in a non-EDT-safe SDK call. I have a thought on how to address the full category of those types of issues. I’ll see if I can get a new build posted either tonight or early tomorrow with those changes.

[Scott Wells]

Actually that issue was just a bug where the wrong call was being made. I've corrected it now and uploaded a new build with that correction. It's certainly possible that there's a similar type of issue as described in my previous comment behind this one, of course.

If you continue to see such an issue -- or really any kind of issue unique to these test builds -- after updating to this build, please provide not just logs/thread dumps, but also any other relevant information such as the host OS (which is admittedly in the logs), anything interesting/different about the project itself, etc., as thus far I'm unable to reproduce these deadlocks locally. Certainly being able to reproduce them -- if they're still occurring -- would allow me to save my test users' valuable time.

Regardless, this test feedback is greatly appreciated!

[Tony White]

Ok, retested with the latest version and it did not hang! yay.

[Scott Wells]

Okay, that’s good to hear! If all test users could align on that build and use it for a bit of additional burn-in, I’d appreciate it. If all goes well, I’m hoping to include these changes in next week’s official build.

[Scott Wells]

New build based on 2.3.1.5. This does not include a fix for the latest hang that Tony has seen, but for those who have been using these builds without issues, it does update it to be based on the latest-and-greatest. I plan to investigate (and hopefully fix) the most recent issue reported by Tony in the next day or two at which point I'll post a new build here.

[Scott Wells]

Okay, here's a build of 2.3.1.5 with the threading changes that I'm hoping puts the observed deadlocks to rest once and for all. Again, anyone helping test, please update to this build, and Tony specifically, it'd be great if you could try it as well since you've been able to reproduce these deadlock scenarios reliably.

[Scott Wells]

Okay, here we are again, and once again, I have (perhaps unrealistically) high hopes for this one.

Here comes the unnecessary explanation, even if just for my own personal records/recollection...

The primary issue in the last several builds based on provided thread dumps has been a call to one particular SDK API that must execute in a read context, and doing so takes out an IDE lock. Doing that while in a Java protected critical section (i.e., synchronized block) was causing deadlocks with access to other protected critical sections on other threads. I've removed that API call from any synchronized block and then resumed the protected critical section when it comes to checking/setting the cached value. It does mean that the API call could occur simultaneously on more than one thread, though it would still be set exactly once, but it importantly removes the chance of a deadlock due to two different locking mechanisms being used simultaneously on the same thread.

Having said that, I'd ask that test users -- specifically Tony -- update to this build and see if it does or does not resolve the remaining deadlocks.

Thanks yet again!

[Arsenii Skvortsov]

Thanks for the explanation, Scott!

I’d like to share my approach to a similar problem, maybe you can draw some inspiration from the code. It’s Kotlin and Reactor, but same could be done with Java and CompletableFuture/Supplier. I’m retrieving a credential from the library (it may or may not send an actual request).

    private final val app: ConfidentialClientApplication
    @Volatile
    private var querier: Mono<String>? = null
    fun getToken(): Mono<String> {
        var current = querier
        if (current == null) {
            synchronized(app) {
                current = querier
                if (querier == null) {
                    querier = app
                        .acquireToken(ClientCredentialParameters.builder(setOf(scope)).build())
                        .let { Mono.fromFuture(it) }
                        .doOnSuccess { synchronized(app) { querier = null } }
                    current = querier
                }
            }
        }
        //Kotlin doesn't understand that all writes are synchronized. ☹
        return current!!
    }

‌

[Scott Wells]

Thanks, @Arsenii Skvortsov . That’s not terribly dissimilar to what I’ve done which is effectively the following (pseudocode):

public X findXForY(Y y) {
    X x = null;

    String cacheKey = getCacheKey(y);
    if (!cache.containsKey(cacheKey)) {
        // If we need to do this, we need to do it outside of the sync block to avoid a potential deadlock
        boolean useLockingApi = false;

        synchronized (cache) {
            if (!cache.containsKey(cacheKey)) {
                x = reallyFindXForY(y);
                if (x != null) {
                    cache.put(cacheKey, x);
                } else {
                    // If we haven't found it yet, try to find it via the locking API outside of this sync block
                    useLockingApi = true;
                }
            }
        }

        // If we still need to populate it via the locking API, do so now raw and then populate sync;
        // it's not perfect, but it's required until we replace/remove calls to that API
        if (useLockingApi) {
            // While we won't necessarily avoid searching on multiple threads, we can minimize the chance of
            // setting the value on multiple threads (given that there are two DCL blocks now, of course)
            if (!cache.containsKey(cacheKey)) {
                X lockingApiX = findXForYUsingLockingApi(y);
                synchronized (cache) {
                    if (!cache.containsKey(cacheKey)) {
                        x = lockingApiX;
                        cache.put(cacheKey, x);
                    }
                }
            }
        }
    }

    if (x == null) {
        x = cache.get(cacheKey);
    }

    return x;
}

‌

[Arsenii Skvortsov]

The essential difference is that this findXForY don’t use a “promise”/“querier” object (Mono/CompletableFuture) that can represent a promise to have the value or (already) the value itself. It is crucial to avoid double querying.

[Scott Wells]

That’s right. IC2 is implemented 100% in Java and such promise-type mechanisms simply don’t work well in Java. In this case, the potential extra call to findXForYUsingLockingApi is truly irrelevant (from a CPU time standpoint), and the cache itself is protected from being populated for the same key more than once.

[Tony White]

Hoping not speaking too soon, but have not had another incident of the IDE locking up when doing a large metadata retrieval.

[Scott Wells]

[Scott Wells]

New build based on 2.3.1.6.

[Scott Wells]

New build based on 2.3.1.7. For those helping test these changes (my continued thanks!), please update to this build and let me know how it's going.

Given that I haven't heard about any issues in a bit -- and barring any newly-reported issues -- I'm planning to include these changes in next week's official build.

[Scott Wells]

New build based on 2.3.1.8 which is a minor hotfix update.

[Arsenii Skvortsov]

I think with this variant (I mean the entire beta rather than the last version) I more often see an error highlighted that refers to the old version code. E.g. I started typing Set<Id>, then changed my mind to Map<Id, SObject> x = new Map<Id, SObject>();, and I get an error “Comparison arguments must be compatible types: Map, Set”. x.put(...,...) later is not highlighted.

Editing the line, opening/closing the file does not help. Reopening project helps.

My temporal settings:
Editor → Code Editing → Autoreparse delay: 0; Tooltip delay: 500.
Advanced Settings → Project-Wide Analysis: Analysis delay after change: 1000.

I’m using IlluminatedCloud2-2.3.1.8-b01.

[Scott Wells]

Arsenii, I just tried reproducing this and wasn’t able to do so. Just to confirm the steps, here’s what I did:

1. Typed Set<Id> as the beginning of a new statement.
2. Backspaced to remove that partial statement.
3. Typed Map<Id, SObject> x = a new Map<tab-to-complete-the-parameterized-type> which resulted in Map<Id, SObject> x = new Map<Id, SObject>();. Note that I also tried it explicitly without the auto-completion, though.
4. Typed <Enter> followed by x. and auto-completion correctly showed variant completions of Map. including isEmpty(), get(key), put(key, value), etc.

Is there some other set of steps that reproduces this behavior for you? It certainly sounds like the original expression type is being cached somewhere beyond its appropriate lifespan, but these changes shouldn’t have affected expression type evaluation/caching/retention, at least not directly.

[Arsenii Skvortsov]

You might want to try erasing just Set, but the problem doesn’t happen constantly, it's just that from time to time my fingers happen to outrace some process. ☺

It might add to the problem that I run Ultimate with Python, and a bunch of Web/Java/Kotlin plugins enabled.

And you are correct, I can use x correctly afterwards, it’s just the error highlight isn’t going anywhere.

[Tony White]

Is this error related to this update?

com.intellij.diagnostic.PluginException: ActionUpdateThread.OLD_EDT is deprecated and going to be removed soon. 'ua.t3hnar.plugins.cmdsupport.actions.RunCmdShellAction' must override getActionUpdateThread() and chose EDT or BGT. See ActionUpdateThread javadoc. [Plugin: CMD Support]
at com.intellij.diagnostic.PluginProblemReporterImpl.createPluginExceptionByClass(PluginProblemReporterImpl.java:23)
at com.intellij.diagnostic.PluginException.createByClass(PluginException.java:90)
at com.intellij.diagnostic.PluginException.reportDeprecatedUsage(PluginException.java:125)
at com.intellij.openapi.actionSystem.ActionUpdateThreadAware.getActionUpdateThread(ActionUpdateThreadAware.java:21)
at com.intellij.openapi.actionSystem.AnAction.getActionUpdateThread(AnAction.java:199)

[Scott Wells]

Tony, no, based on ua.t3hnar.plugins.cmdsupport in the stack trace, that appears to be from this plugin and not IC2.

[Tony White]

Sorry, should have engaged the brain, saw intellij and brain linked it to IC2 ;)

[Scott Wells]

No issues. Given that I'm trying to make sure these changes are as rock solid as possible before releasing them into the wild, I'd rather hear about more potential issues than less. Keep 'em coming and we'll sort through them.

[Scott Wells]

Arsenii, I’ve tried to reproduce it by typing Set<Id> and then removing Set and replacing it with Map followed by populating a second type parameter, then completing the rest of the statement. I’m not seeing any errant behavior.

Have you confirmed that this is not at all reproducible on the official build but is reproducible only on the builds I’m posting here? It sounds like either way there’s an issue, but I’m trying to figure out how to approach troubleshooting it and whether it’s related to the changes being tested here or something else that already exists in the official builds as well.

[Arsenii Skvortsov]

I’ve experienced “stuck error highlight” in the official build too. But I feel, in this build it happens more often. Still not too often to become annoying.

[Scott Wells]

Gotcha. If you can find a pattern that seems to reproduce the behavior pretty reliably for you, ideally in a project that can be shared because perhaps the size and complexity of the project has something to do with it(?), please provide that info/project so that I can hopefully reproduce it locally and investigate the root cause.

[Scott Wells]

New build based on 2.3.1.9, the update for Summer '24 / API v61.0. I'd mentioned that the next official build would likely include these same changes, but I'd also overlooked the need for a platform release update. If all goes well, next week will move these threading changes into the official builds.

[Arsenii Skvortsov]

With this update I quite often see one more problem that looks like a race condition: at times there is no message to synchronize API version, sometimes there are two. I was opening a set of Apex Test files via Ctrl+N (Go to class), Ctrl+V (paste name from SF Test report), Alt+Tab to SF to Ctrl+C another name. Files had API version <61 on 61-enabled connection.

[Scott Wells]

Arsenii, that’s actually a known issue in the JetBrains IDE with file-level annotations. Take a look at #2551 for some details, and the linked JetBrains YouTrack issues for even more details. They’d marked it as resolved in 2024.1.2, but it’s definitely still present. I’ve seen it, as have numerous IC2 users, but it’s also still being reported on the linked JetBrains issues by users of other plugins.