Clear overlay state vars in lsp-ui-doc--hide-frame

[Lenbok]

Fixes #695

Minimal fix. See #711 for more discussion. Note that I also tested @kiennq speculation that this method may have a performance penalty compared to using an additional overlay-buffer condition inside lsp-ui-doc--glance-hide-frame, and found that this PR actually improves the speed of doc display by a significant factor. Profiling function:

(defun my-time-lsp-ui-doc-show()
  (interactive)
  (profiler-start 'cpu)
  (message "Starting")
  (let ((start-time (current-time))
        (iterations 1000))
    (dotimes (counter iterations)
      (lsp-ui-doc-show)
      (lsp-ui-doc-hide))
    (let* ((elapsed (float-time (time-subtract (current-time) start-time)))
           (iteration-time (/ elapsed iterations)))
      (profiler-stop)
      (message "Finished %d iterations in %.3fs (%.3fs / iteration)" iterations elapsed iteration-time))))

Master:

Finished 1000 iterations in 16.885s (0.017s / iteration)

This PR:

Finished 1000 iterations in 9.936s (0.010s / iteration)

[kiennq]

I don't think that profiling is correct/applicable for this function since a lot of things are involved here (json-rpc perf, io queue pipe when the profiling is running, the profiling implementation details ...). For example, using the same profiling function on my env provide these data:

move: Finished 1000 iterations in 11.041s (0.011s / iteration)
make: Finished 1000 iterations in 13.098s (0.013s / iteration)
master:  Finished 1000 iterations in 17.059s (0.017s / iteration)

So the version of using overlay-buffer + move is better than the version of set to nil + make a new overlay.

I think you want to benchmark the perf of making a new overlay compare to move overlay + overlay-buffer here. A simple benchmark like this would be sufficient

(setq ov (make-overlay 1 10))
#<overlay from 1 to 10 in *scratch*>

(benchmark-run 100000 (progn
                        (delete-overlay ov)
                        (setq ov nil)
                        (setq ov (make-overlay 10 20))))
(0.061306730000000004 0 0.0)

(benchmark-run 100000 (progn
                        (delete-overlay ov)
                        (move-overlay ov 10 20)
                        (overlay-buffer ov)))
(0.034729481 0 0.0)

The move + overlay-buffer performs better.

Since overlay is being really deleted on garbage collection in Emacs, creating too much overlay can make the garbage collecting happens more frequently and sometimes slow down the whole Emacs.

[Lenbok]

Profiling the actual function is more useful than a toy example (as in your progn) as it demonstrates a more realistic usage pattern. I would like to also trigger redisplay to make it even more realistic - do you know how to do that? If it turns out the difference is negligible in the real-world scenario due to other factors dominating, then we are treading into premature optimization territory).

In case the profiler itself was causing problems as you suggested, I repeated my run using benchmark-run instead of the profiler and got essentially the same results: Master -> 18.7s This PR -> 9.7s

What is your move version? The existing implementation on master supposedly already re-uses the overlay, does that mean it's actually broken?

[kiennq]

What is your move version? The existing implementation on master supposedly already re-uses the overlay, does that mean it's actually broken?

I actually realize that I've changed in the wrong place by the' move' version so it would not take effect. So I retract the move version that I mentioned and use the master version instead. Here is the benchmark version on the master (which use move overlay) and your PR.

(defun my-time-lsp-ui-doc-show()
  (interactive)
  (message "result: %s"
           (benchmark-run 1000
             (progn
               (lsp-ui-doc-show)
               (lsp-ui-doc-hide)))))
my-time-lsp-ui-doc-show

master: result: (9.237509808999999 4 0.6467752440000001)
pr: result: (11.377403877 5 0.7797696419999998)

Your PR which is resetting the overlay took more time and more garbage collection as well.

Boiling down the difference, it's mostly just the difference in using the move-overlay or make-overlay so my toy benchmark is actually working better here.

Simulating the "real" scenario as you said is being affected by a lot of external causes, for example, LSP perf might degrade the longer it runs, the same for Emacs memory.

[Lenbok]

Do you have display-line-numbers-mode enabled? I have been looking at why master is so slow for me compared to this PR, and that seems to be the difference. The overlay re-use seems incompatible with display-line-numbers-mode. Can you try to replicate?

[Lenbok]

BTW, my numbers using your benchmark function:

Current PR
Display-Line-Numbers mode enabled in current buffer
result: (9.8282039 9 1.2140667350000003)
Display-Line-Numbers mode disabled in current buffer
result: (10.308526724 9 1.5485936239999996)

Re-use overlay (master)
Display-Line-Numbers mode enabled in current buffer
result: (18.412163156000002 9 1.321076187)
Display-Line-Numbers mode disabled in current buffer
result: (9.649574025 9 1.2580851540000002)

[kiennq]

Do you have display-line-numbers-mode enabled? I have been looking at why master is so slow for me compared to this PR, and that seems to be the difference.

Interesting. I do have display-line-numbers-mode enabled. Which source file are you using to test? I've been using this test file with cursor is placed in <|>

function fib(num) {
    if (num <= 1) {
        return num;
    }

    const result = fib(num - 1) + fib(num - 2);
    console.log(`fib(${num}): ${result}`);
    return result;
}

const f = fib(5);

const cp = require("child_process");
cp.s<|>pawn("./a.cmd");

class Foo {
    size = 4;
    soo() { }
    ["so so"]() { }
}

let a = new Foo();

[Lenbok]

I don't have javascript set up, I did do one of the server installs and there are no real docs for spawn (I assume that needs setting up some additional libraries). With the cursor on require there is a doc of a couple of lines. With that there are no significant differences when running the benchmarking on master with/without line numbers.

I suspect that since it's related to display-line-numbers-mode it may depend on the length of the documentation and/or the source file and how they relate to the overlay. For the previous benchmarking I've been using openscad (a file about 80 lines long, and a function doc about 10 lines long) and I also tried some python that was slower overall and when display-line-numbers-mode is active it is slower but by only a few seconds. Both of those were bringing up docs from other source files, so not so easy to share. I did an experiment of computing the inline window width once instead of calling lsp-ui-doc--inline-window-width for every line of the doc and that made most of the difference go away:

modified   lsp-ui-doc.el
@@ -745,8 +745,9 @@ FN is the function to call on click."
     string))

 (defvar-local lsp-ui-doc--inline-width nil)
+(defvar-local lsp-ui-doc--inline-window-width nil)

-(defun lsp-ui-doc--inline-window-width nil
+(defun lsp-ui-doc--calc-inline-window-width nil
   (- (min (window-text-width) (window-body-width))
      (if (bound-and-true-p display-line-numbers-mode)
          (+ 2 (line-number-display-width))
@@ -754,11 +755,10 @@ FN is the function to call on click."
      1))

 (defun lsp-ui-doc--inline-zip (s1 s2)
-  (let* ((width (lsp-ui-doc--inline-window-width))
-         (max-s1 (- width lsp-ui-doc--inline-width 2)))
+  (let* ((max-s1 (- lsp-ui-doc--inline-window-width lsp-ui-doc--inline-width 2)))
     (truncate-string-to-width
      (concat (truncate-string-to-width s1 max-s1 nil ?\s) s2)
-     width nil ?\s)))
+     lsp-ui-doc--inline-window-width nil ?\s)))

 (defun lsp-ui-doc--inline-padding (string len)
   (let ((string (concat " " string (make-string (- len (string-width string)) ?\s) " ")))
@@ -768,6 +768,7 @@ FN is the function to call on click."
 (defun lsp-ui-doc--inline-faking-frame (doc-strings)
   (let* ((len-max (-max-by '> (-map 'string-width doc-strings))))
     (setq lsp-ui-doc--inline-width len-max)
+    (setq lsp-ui-doc--inline-window-width (lsp-ui-doc--calc-inline-window-width))
     (--map (lsp-ui-doc--inline-padding it len-max) doc-strings)))

 (defun lsp-ui-doc--inline-untab (string)

[Lenbok]

There are too many other things broken with lsp-ui-doc, it needs wider changes. Closed in favor of #711