Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debounce refreshes of the info-mode #11

Merged
merged 1 commit into from
Jun 3, 2022

Conversation

bollu
Copy link
Contributor

@bollu bollu commented May 28, 2022

This prevents emacs from lagging when scrolling
through a file that has many #print messages, or when
there are many errors.

This was discovered when writing this lean file with many errors.

The profiler output (use M-x profiler-find-profile to view is below:

   3832  38% + catch
        3632  36% + #<lambda -0x1e43a018eefeb959> ;; magit closure
         325   3% + linum-update-window
         279   2% + complete-with-action
         254   2% + while
         227   2% + eieio-oref
         116   1% + magit-insert-child-count
 ...

The debounce implementation was taken from lsp-lens

Copy link
Contributor

@Kha Kha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this should really help with responsiveness! However, could you please adhere to repos' commit message conventions?

lean4-info.el Outdated
Comment on lines 137 to 138
;; That is, we maintain the timing invariant that the time lean4-info-buffer-redisplay
;; is invoked is at least (debounce-interval-seconds) after the last redisplay request.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That logic looks kind of wrong to me; we should refresh at most duration-seconds (you used three different names btw) after any request. Otherwise with sufficiently frequent requests such as from building dependent files, we would not show anything at all for quite a while.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Kha Ah, I didn't know that there are cases where we get many requests. I was under the impression that a single request is generated per user movement command.

The current code "delays" rendering by duration-seconds, to wait for more changes that might come in. Do you suggest we add another timer (say, debounce-upper-bound-duration) which is a somewhat long duration such as 0.5sec? This is so that in the case where we get a large sequence of requests, we do render some information once in a while.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do you think one refresh every 0.1s is too often? Because in that case one timer would suffice. But I'm okay with either solution.

@bollu
Copy link
Contributor Author

bollu commented May 29, 2022

@Kha To check I understand commit conventions, I should call this feat: debounce refreshes of info-mode, correct?

@Kha
Copy link
Contributor

Kha commented May 30, 2022

@Kha To check I understand commit conventions, I should call this feat: debounce refreshes of info-mode, correct?

Yeah, looks appropriate.

This prevents emacs from lagging when scrolling
through a file that has many `#print` messages, or when
there are many errors.

;; Debouncing
;; ~~~~~~~~~~~
;; We want to update the Lean4 info buffer as seldom as possible,
;; since magit-section is slow at rendering. We
;; wait a small duration ('debounce-delay-sec') when we get a
;; redisplay request, to see if there is a redisplay request in the
;; future that invalidates the current request (debouncing).
;; Pictorially,
;; (a) One request:
;; --r1
;; --r1.wait
;; ----------r1.render
;; (b) Two requests in quick succession:
;; --r1
;; --r1.wait
;; --------r2(cancel r1.wait)
;; --------r2.wait
;; ---------------r2.render
;; (c) Two requests, not in succession:
;; --r1
;; --r1.wait
;; ---------r1.render
;; ------------------r2
;; ------------------r2.wait
;; -------------------------r2.render
;; This delaying can lead to a pathological case where we continually
;; stagger, while not rendering anything:
;; --r1
;; --r1.wait
;; --------r2(cancel r1.wait)
;; --------r2.wait
;; --------------r3(cancel r2.wait)
;; ---------------r3.wait
;; ---------------------r4(cancel r3.wait)
;; ---------------------...
;; We prevent this pathological case by keeping track of when
;; when we began debouncing in 'lean4-info-buffer-debounce-begin-time'.
;; If we have been debouncing for longer than
;; 'lean4-info-buffer-debounce-upper-bound-sec', then we
;; immediately write instead of debouncing;
;; 'max-debounces' times. Upon trying to stagger the
;; 'max-debounces'th request, we immediately render:
;; begin-time:nil----t0----------------nil-------
;;            -------r1                |
;;            -------r1.wait           |
;;            -------|-----r2(cancel r1.wait)
;;            -------|-----r2.wait     |
;;            -------|-----------r3(cancel r2.wait)
;;            -------|-----------r3.wait
;;            -------|-----------------r4(cancel r3.wait)
;;            -------|-----------------|
;;                   >-----------------<
;;                   >longer than 'debounce-upper-bound-sec'<
;;            -------------------------r4.render(FORCED)
@bollu bollu force-pushed the debounce-info-mode-refresh branch from 9a7749d to e6738ea Compare June 2, 2022 21:05
@bollu
Copy link
Contributor Author

bollu commented Jun 2, 2022

@Kha I request a review, as I've changed the rendering code to add an upper bound to the debounce.

@Kha Kha merged commit c10def3 into leanprover-community:master Jun 3, 2022
@Kha
Copy link
Contributor

Kha commented Jun 3, 2022

@Kha To check I understand commit conventions, I should call this feat: debounce refreshes of info-mode, correct?

I see that didn't work out 😅 . But the code looks good, thanks!

@bollu
Copy link
Contributor Author

bollu commented Jun 3, 2022

@Kha I am confused, isn't the commit titled feat: debounce refreshes of info mode? It says so here:

c10def3

@Kha
Copy link
Contributor

Kha commented Jun 3, 2022

Yes, because I fixed it in the merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants