minad / corfu

:desert_island: corfu.el - COmpletion in Region FUnction
GNU General Public License v3.0
1.06k stars 42 forks source link

corfu: Reduce number of auto-timers created #404

Closed karthink closed 6 months ago

karthink commented 6 months ago

If corfu--auto-timer is running, we can reset its time in the post command hook instead of cancelling the timer and creating a new one.

Here's a rough calculation: With 8 byte integers (calculated from most-possible-fixnum), the corfu--auto-timer object is a vector of approximately:

  48 bytes for various times
+ (size of function address)
+ (size of window address)
+ (size of current buffer address)
+ (8 bytes from buffer-modified-tick)
+ (8 bytes from point)

Let's say the function, window and current buffer addresses are word size, or 8 bytes each. The timer object is then at least 88 bytes.

This timer is canceled and recreated each time in post-command-hook via corfu--auto-post-command when corfu isn't triggered, let's say 60 times/min when typing. That's about 5 Kb of garbage/min we can avoid. corfu--auto-post-command should also run slightly faster when corfu doesn't pop up, i.e. for most non-typing commands.

This might be a bit of a micro-optimization, but it doesn't appear to have any downside and the code is a little shorter too, so I thought I'd make a PR.

karthink commented 6 months ago

Update: Oh, never mind, this won't work since the result of (corfu--auto-tick) at the time that Corfu should pop up will be different from (corfu--auto-tick) from when the timer was created. I didn't notice any issues when testing it but I'm guessing a sufficiently quick context (window, buffer) change can cause inconsistency.

If you don't see an easy way around this problem it's probably not worth optimizing this and I can close the PR.

minad commented 6 months ago

Hi Karthik,

thanks for this! I usually appreciate such micro optimizations and I already tried to optimize my packages to avoid creating garbage. In this case I don't think we should do it, since 300K/h is a tiny rate, and given that the correct code won't be simpler. See https://github.com/minad/corfu/tree/reuse-timer for an untested, but probably more correct implementation, I think. But timers will still get cancelled there, so we won't win as much.

As an alternative one could either set the timer to a large (~infinite) delta instead of cancelling and this way keep it alive all the time. I think it would work, but it would also feel like a hack, which is not a good sign. Using a hack would absolutely make sense if the optimization would lead to a noticeable improvement for example.

Generally I already try to do the following:

1 Use the memory profiler. It will show allocation hotspots if I understand correctly, and this translates to either live memory or garbage collector pressure.

  1. Avoid allocating large objects like strings, vectors, hash tables. Cache them where possible.
  2. Avoid allocating costly objects like markers, overlays, buffers. These objects are costly since they create slow down independent of their memory usage, super linear scaling in some lookups etc. Maybe timers belong to this class? However creating on the order of one of these objects per key press (and disposing it, no accumulation) should usually be okay.

One should note that Elisp is not a language which is optimized well to allow for zero allocation. For list manipulations, some functions are mutating, but for strings the situation is worse. There we can only manipulate the properties and mutate characters with aset, which has an unpredictable cost, since the string may still get reallocated if a multibyte character is inserted.

Anyway, if you find any such spots in my packages, which should and could easily be optimized, please let me know. I am very much interested in reducing allocations when possible, since the GC is not a strength of Emacs.

minad commented 6 months ago

corfu--auto-post-command should also run slightly faster when corfu doesn't pop up, i.e. for most non-typing commands.

I investigated this a little more, and what you said here seems relevant. For example when repeatedly pressing a DEL a DEL a DEL a DEL a DEL..., the PCH appears in the profile (not very prominently, but still). So I decided to indeed reuse the timer object. I've also found a sufficiently simple solution by relying more on the lower level timer APIs, see https://github.com/minad/corfu/commit/aec861d832236dd35969c50930f1c9f6ea5fb122. I would appreciate if you give it a thorough test. Maybe you could also take a quick look if the code makes sense like this for you. Thanks!

EDIT: If we also include https://github.com/minad/corfu/commit/1385e3d50c232f6cbea690e9a2252d703e365269, the changed code has equally many lines as the one without timer reuse. So you haven't been wrong about the code size. ;)

karthink commented 6 months ago

Anyway, if you find any such spots in my packages, which should and could easily be optimized, please let me know. I am very much interested in reducing allocations when possible, since the GC is not a strength of Emacs.

Will do. I don't have a very good sense of what will cause performance issues in elisp. I understand it in the abstract (lots of allocation = bad) and I understand some details (nreverse has a dedicated byte-code op), but not well enough to put these ideas into practice when looking at or writing code. I'm limited to looking at profiler/elp output and guessing.

I've also found a sufficiently simple solution by relying more on the lower level timer APIs, see https://github.com/minad/corfu/commit/aec861d832236dd35969c50930f1c9f6ea5fb122. I would appreciate if you give it a thorough test. Maybe you could also take a quick look if the code makes sense like this for you.

Indeed, your solution avoids ever setting corfu--auto-timer to nil, so it's better than what I had in mind. I don't expect to notice a big difference in responsiveness because of the other functions in my PCH, but I'll certainly test the behavior and let you know if there are any inconsistencies.


More generally, I've been seeing and writing debounce timers in elisp quite often in the last few months. Seems like a common enough feature for interactive applications. Do you think Emacs could provide a debounce feature? The behavior is subtly different from idle-timers in a way that matters, and it can be well optimized so the modifications you had to find above don't have to be rediscovered.

I wrote some code to debounce commands from other packages and ended up needing it quite often.

minad commented 6 months ago

More generally, I've been seeing and writing debounce timers in elisp quite often in the last few months. Seems like a common enough feature for interactive applications. Do you think Emacs could provide a debounce feature? The behavior is subtly different from idle-timers in a way that matters, and it can be well optimized so the modifications you had to find above don't have to be rediscovered.

Debouncing is certainly important and I use it at multiple places in my packages (Corfu, Consult, Jinx). I am not sure however if some kind of special API is really needed. The advices in your library seem dangerous. I assume that for most scenarios one has to write specialized code anyway and for this the current timer API doesn't seem too bad. See for example corfu--auto-post-command, consult--async-throttle or consult--async-refresh-timer.

I think discipline for package authors is quite important. One could either document these things better and/or add better APIs. Mode line constructs and post command hooks can quickly lead to latency issues.

karthink commented 6 months ago

The advices in your library seem dangerous. I assume that for most scenarios one has to write specialized code anyway and for this the current timer API doesn't seem too bad.

Yeah, by API I didn't mean the advice-based debouncing from the library, that's basically a way for me to limit mode-line elements and other automatic behavior in my config.

I was thinking of something more along the lines of run-with-debounce that is similar to run-with-timer, but accepts a couple of additional arguments for the debounce time and a predicate to reset the timer. I haven't given it more thought than that. I'm not sure when the predicate would be called, exactly.

I think discipline for package authors is quite important. One could either document these things better and/or add better APIs. Mode line constructs and post command hooks can quickly lead to latency issues.

Providing an easier way for package authors to debounce or throttle their interactive elements will make it easier to exercise this discipline as well. That said, any author who's aware of Emacs' timer API is probably also capable of using it to implement a debounce, modulo the micro-optimizations that you just implemented for Corfu.

minad commented 6 months ago

I would have to see some usage examples of such an API. It may be useful. The best approach to check is to use it in real packages and apply it to real problems.

However in most cases I think one needs a specialized and optimized solution. For example I recently migrated to EXWM and implemented my own system statusbar as part of the tab-bar. There I also debounce almost everything using some special logic. Most elements are updated every 10s, some only every minute, and some on every redisplay.