Fix stale hover reuse for unchanged regions

[betamaxbandit]

LSPTextHover reused a completed hover request when the viewer and hover region were unchanged. As a result, a repeated hover over the same region could return stale content and avoid issuing a fresh textDocument/hover request.

Fixes #1514

[rubenporras]

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

[betamaxbandit]

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

Hi, good question. No it is not always recomputed.
The previous hover request is still reused when it is still in progress and the viewer/region are unchanged. The change is specifically that a completed request is no longer reused indefinitely for the same region.

I've updated the commit message appropriately.

Cheers John

[betamaxbandit]

Hi @rubenporras ,

I see the following failure for my PR:

continuous-integration/jenkins/pr-merge — This commit cannot be built

I'm not sure what I need to do, if anything. Is it simply waiting for a committer to approve the run?

[betamaxbandit]

Hi @rubenporras ,

I see the following failure for my PR:

continuous-integration/jenkins/pr-merge — This commit cannot be built

I'm not sure what I need to do, if anything. Is it simply waiting for a committer to approve the run?

Hi @rubenporras ,
can you help me progress this please?
Cheers John

[FlorianKroiss]

I haven't reviewed the code (yet), but I approved the workflow runs

[rubenporras]

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

Hi, good question. No it is not always recomputed. The previous hover request is still reused when it is still in progress and the viewer/region are unchanged. The change is specifically that a completed request is no longer reused indefinitely for the same region.

I've updated the commit message appropriately.

Cheers John

I think this will trigger many unneeded recomputations. Could we do it so that we invalidate the last computed hover request only if the editor looses focus?

That should cover the case of external resources and be less agressive on the computation side.

[betamaxbandit]

What are the new conditions to avoid reusing a previoulsy computed hover request? Is it now always recomputed? Could you specify if as part of the commit / PR message?

Hi, good question. No it is not always recomputed. The previous hover request is still reused when it is still in progress and the viewer/region are unchanged. The change is specifically that a completed request is no longer reused indefinitely for the same region.
I've updated the commit message appropriately.
Cheers John

I think this will trigger many unneeded recomputations. Could we do it so that we invalidate the last computed hover request only if the editor looses focus?

That should cover the case of external resources and be less agressive on the computation side.

Hi @rubenporras ,

I like the goal of optimising the number of recomputations, and your idea of triggering this on editor focus lost, but I'm skeptical whether its worth it or always beneficial.

For instance, consider the case where the editor focus is not lost, but an external event causes the need for a recomputation. That could happen if the user starts a project build using a keybaord shortcut (ctrl+B) and this causes header files to be built which generate new values in the external resources.

Also I did some tests in vscode and see that it always generates a hover request without trying to optimise the request - see below.

Would you be willing to reconsider this please?

==
In vscode*, hovering testA twice, genrates 2 identical sets of clangd hover requests.

testA hover 1

V[17:20:34.014] <<< {"id":"clangd-proxy/internal/51","jsonrpc":"2.0","method":"textDocument/definition","params":{"position":{"character":10,"line":3},"textDocument":{"uri":"file:///c%3A/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}}

I[17:20:34.014] <-- textDocument/definition("clangd-proxy/internal/51")
V[17:20:34.014] ASTWorker running Definitions on version 1 of c:\Users\a5107948\git\CrossProbe_Williamb\717595E8-4B06-45AC-BA88-3AA846007E1C\src\test.c
I[17:20:34.014] --> reply:textDocument/definition("clangd-proxy/internal/51") 0 ms
V[17:20:34.014] >>> {"id":"clangd-proxy/internal/51","jsonrpc":"2.0","result":[{"range":{"end":{"character":13,"line":3},"start":{"character":8,"line":3}},"uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}]}

I[17:20:34.014] --> textDocument/clangd.fileStatus
V[17:20:34.014] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}

V[17:20:34.015] <<< {"id":196,"jsonrpc":"2.0","method":"textDocument/hover","params":{"position":{"character":10,"line":3},"textDocument":{"uri":"file:///c%3A/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}}

I[17:20:34.015] <-- textDocument/hover(196)
V[17:20:34.015] ASTWorker running Hover on version 1 of c:\Users\a5107948\git\CrossProbe_Williamb\717595E8-4B06-45AC-BA88-3AA846007E1C\src\test.c
I[17:20:34.017] --> reply:textDocument/hover(196) 1 ms
V[17:20:34.017] >>> {"id":196,"jsonrpc":"2.0","result":{"contents":{"kind":"markdown","value":"### variable `testA`  \n\n---\nType: `int`  \nValue = `100 (0x64)`  \n\n---\n```cpp\n// In test\nint testA = _A_\n```"},"range":{"end":{"character":13,"line":3},"start":{"character":8,"line":3}}}}

I[17:20:34.017] --> textDocument/clangd.fileStatus
V[17:20:34.017] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}

testA hover 2

V[17:23:52.854] <<< {"id":"clangd-proxy/internal/52","jsonrpc":"2.0","method":"textDocument/definition","params":{"position":{"character":10,"line":3},"textDocument":{"uri":"file:///c%3A/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}}

I[17:23:52.854] <-- textDocument/definition("clangd-proxy/internal/52")
V[17:23:52.856] ASTWorker running Definitions on version 1 of c:\Users\a5107948\git\CrossProbe_Williamb\717595E8-4B06-45AC-BA88-3AA846007E1C\src\test.c
I[17:23:52.856] --> reply:textDocument/definition("clangd-proxy/internal/52") 2 ms
V[17:23:52.856] >>> {"id":"clangd-proxy/internal/52","jsonrpc":"2.0","result":[{"range":{"end":{"character":13,"line":3},"start":{"character":8,"line":3}},"uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}]}

I[17:23:52.856] --> textDocument/clangd.fileStatus
V[17:23:52.856] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}

V[17:23:52.857] <<< {"id":197,"jsonrpc":"2.0","method":"textDocument/hover","params":{"position":{"character":10,"line":3},"textDocument":{"uri":"file:///c%3A/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}}

I[17:23:52.857] <-- textDocument/hover(197)
V[17:23:52.857] ASTWorker running Hover on version 1 of c:\Users\a5107948\git\CrossProbe_Williamb\717595E8-4B06-45AC-BA88-3AA846007E1C\src\test.c
I[17:23:52.859] --> reply:textDocument/hover(197) 2 ms
V[17:23:52.859] >>> {"id":197,"jsonrpc":"2.0","result":{"contents":{"kind":"markdown","value":"### variable `testA`  \n\n---\nType: `int`  \nValue = `100 (0x64)`  \n\n---\n```cpp\n// In test\nint testA = _A_\n```"},"range":{"end":{"character":13,"line":3},"start":{"character":8,"line":3}}}}

I[17:23:52.860] --> textDocument/clangd.fileStatus
V[17:23:52.860] >>> {"jsonrpc":"2.0","method":"textDocument/clangd.fileStatus","params":{"state":"idle","uri":"file:///c:/Users/a5107948/git/CrossProbe_Williamb/717595E8-4B06-45AC-BA88-3AA846007E1C/src/test.c"}}

*: vscode using extension "llvm-vs-code-extensions.vscode-clangd" and settings.json set with:
"clangd.arguments": [
"--log=verbose"

[betamaxbandit]

Would you be willing to reconsider this please?

Hi @rubenporras

Any thoughts on this please?

Cheers John

[rubenporras]

Would you be willing to reconsider this please?

Hi @rubenporras

Any thoughts on this please?

Cheers John

Hi John,

To me, firing a call every time the mouse moves is not an option.

I think there must be some code that avoids triggering the same computations again and again. I think that focus is probably a good approximation, but I agree it is not perfect. One could also debounce the calls, but that has other defects.

In your tests above there is 3 minutes between calls, so unless you where not doing anything, vscode is also doing some optimization, isn't it?

[FlorianKroiss]

@rubenporras

To me, firing a call every time the mouse moves is not an option.

I don't think that's actually a problem. I observed the following, when testing this branch locally:

• The hover request is only sent after the mouse does not move for a brief period of time. This request is used to get the region for which the hover information is valid.
• Moving the mouse to a different location inside that area does not seem to re-trigger the hover computation, so there should not be a risk that we overwhelm the LS with requests.

In your tests above there is 3 minutes between calls, so unless you where not doing anything, vscode is also doing some optimization, isn't it?

I checked VS Code with a different LS and can confirm that a fresh hover request is sent when the mouse hovers a region, i.e., hover over a method signature -> move mouse outside of editor -> hover same signature again -> two requests are sent in total.

I speculate that this request caching originally comes from the difference between the Eclipse API and the LS API:

• Eclipse first computes the region for which a hover is valid and then computes the actual hover information.
• The LS hover call computes both pieces of information at once. So this caching presumably tried to avoid sending a second request if the previous request already had the required data.
• Depending on how fast the LS responds, we now send at most two requests, which I still think is fine?

Possible improvements:
Maybe we could remove this request caching entirely if we let getHoverRegion return an IRegion implementation which already contains the hoverText, in case the LS responded fast enough.
In getHoverInfo we can then do an instance check on the passed region and immediately return that result. But I'm not sure if this has any other side effects.

[FlorianKroiss]

@betamaxbandit I hope it's ok for you that we went ahead and merged #1544. Can you please verify that this solves your problem and then close this PR?

[betamaxbandit]

@betamaxbandit I hope it's ok for you that we went ahead and merged #1544. Can you please verify that this solves your problem and then close this PR?

Hi @FlorianKroiss & @rubenporras ,
thanks for working on this issue guys. Really appreciate it.
I've pulled the latest PR into my local env and will check it works.
I'll let you know when I've completed my testing.

Cheers John