Possibly fix #113 by reducing the lock contention on `InternalVcsService`

[bric3]

This commit should fix #113 where the ClearLocalMessageHistoryAction.update was causing a PluginException: 316 ms to call on EDT because the InternalVcsService was slow to get created.

The reason is that the creation happened on EDT, but the thread couldn't progress because there was a lock. The idea here is to reduce the contention by moving the heavy computation outside the write lock.

Fixes #113 For more details look at #113

[lppedd]

The comment you left on the other PR was

The issue with getServiceIfCreated, is that it may not create the service at this point. But the same issue could pop in a different place since this service will be needed at some point.

Especially regarding

the same issue could pop in a different place

my opinion is this should be expected. I mean, a service is not generally lightweight, in the sense that it's expected it may require considerable resources to be initialized.

The problem here is related to the combination of service + EDT. Wouldn't it be better to document the service is slow to initialize, and get it only if it's already created? Most of services usage should be in background threads anyway.
Also, wasn't there an option in the XML to have eager initialization? I may be recalling wrong stuff 🤔 Edit: probably eager init wouldn't work here because we depend on other platform services.

This change might work but it is based on what you can observe on your specific machine.

---

This is the code for getServiceIfCreated, should work fine I think.

https://github.com/JetBrains/intellij-community/blob/852c01851039da44a484b605ce2be8e40de7d57c/platform/service-container/src/com/intellij/serviceContainer/ComponentManagerImpl.kt#L691-L705

[lppedd]

Let's say we go the route I proposed. My concern now is: the first time update is called we set isEnabled to false because the service is null. Is update called again at some point in the future? Do we have to register some kind of listener and update the presentation ourselves?

[lppedd]

@bric3 I've come up with another alternative, see https://github.com/lppedd/idea-conventional-commit/commit/f91dff13b03942d08bcd8ab34d5e9d69cf676e4e

Let me know what you think about this.

[bric3]

I believe this approach helps service loading. However, this won't help the lock contention problem.

After analysis of the initial issue, InternalRecentCommitsService is actually the service that is being initialized, but to initialize it needs InternalVcsService which turns out be already created (the IJ returns the instance) but also turns out it's still initializing.

And the problem was that InternalVcsService initializing thread was holding the write lock, while the InternalRecentCommitsService initializing thread was trying to read from InternalVcsService and waited for the read lock because service InternalVcsService was created but still initializing.

If the locks were held for a shorter time, this wouldn't happen.

In f91dff13b03942d08bcd8ab34d5e9d69cf676e4e putting the whole thing on the pooled thread won't change the locking behavior, the fetch* method are long operations that would still hold the write lock resulting in reading thread being parked.

[lppedd]

Sounds like a fair point. Last question: given the scope of the two locks is reduced to a single assignment, couldn't we just declare the two fields as volatile and call it a day?

[bric3]

Indeed, I was thinking about that too. I didn't want to introduce too many changes as I was unsure about future designs in this code.

[lppedd]

I think the only change I'll make in the future is regarding the meaning of "recent commits". Currently I just take N elements, but recent should also mean inside of a certain timespan, e.g., a couple of days. VCS stuff should not be included in that, but anyway, we can change as many times as we want, no problem.

Oh btw, if you're going to test if your changes work again, remember to rebase on master.

[bric3]

looks good for now