Implement caching for diff in IntelliJ extension #4753
Conversation
ferenci84
requested review from
Patrick-Erichsen
and removed request for
a team
✅ Deploy Preview for continuedev canceled.
|
There was a problem hiding this comment.
Hey @ferenci84 , thanks for the additional contribution here!
I'm curious what prompted you to build this out - have you been having issues with the current getDiff? Looking at the code I can see how it would be quite expensive to compute, so a cache here makes sense to me, but just curious if you've been having issues or if it's just a part of the codebase you noticed could be improved.
Regarding my review - I was a bit nitpicky here because this feels like an important part of the codebase to get right, so I think if we want to build out a cache for diffs we should make sure it's rock solid since we're introducing more complexity.
| * Updates the timestamp when a file is saved | ||
| */ | ||
| override fun updateLastFileSaveTimestamp() { | ||
| println("Updating last file save timestamp") |
There was a problem hiding this comment.
| println("Updating last file save timestamp") |
I think this will be too noisy of a log
| @@ -110,6 +129,16 @@ class IntelliJIDE( | |||
| } | |||
|
|
|||
| override suspend fun getDiff(includeUnstaged: Boolean): List<String> { | |||
| println("Getting diff, cache timestamp: ${diffCache?.timestamp}, current timestamp: $lastFileSaveTimestamp") | |||
There was a problem hiding this comment.
| println("Getting diff, cache timestamp: ${diffCache?.timestamp}, current timestamp: $lastFileSaveTimestamp") |
Similarly, I think this will be too noisy
|
|
||
| // Check if we have a valid cache entry | ||
| if (diffCache != null && diffCache!!.timestamp == lastFileSaveTimestamp) { | ||
| println("Using cached diff") |
There was a problem hiding this comment.
| println("Using cached diff") |
| } | ||
|
|
||
| // If no cache hit, compute the diff | ||
| println("Computing fresh diff") |
There was a problem hiding this comment.
| println("Computing fresh diff") |
...c/main/kotlin/com/github/continuedev/continueintellijextension/continue/IdeProtocolClient.kt
Outdated
Show resolved
Hide resolved
...lij/src/main/kotlin/com/github/continuedev/continueintellijextension/continue/IntelliJIde.kt
Outdated
Show resolved
Hide resolved
|
Hi @Patrick-Erichsen, Thank you for thoughtful review. I have made the requested modifications. Regarding cache size, this stores only the very last value, so the size is already limited, it's not a full-featured cache, anything more complicated would be overkill for this case.
I made a resolution of issue #4130 for VS code with a default 10s cache for platforms with no implementation, and recently I'd been getting a message every few days that the problem presents (too many git processes are started at the same time), and it's so serious that they cannot use the extension at all. This is not something that comes up with simple test scenarios, but in real-world case where you have lot of staging modifications and potentially large diff (think complied js files that are technically not binary, but are very large and diff computation is slow). |
ferenci84
force-pushed
the
intellij_cache_diff
branch
from
249b61e to
af26eae
Compare
Description
Caches diff for IntelliJ plugin.
Issue: #4130
Checklist
Testing instructions
Use autocomplete in a testing environment under IntelliJ.
IDE Logs tab contains logs about caching:
