STM debouncer

[pepeiborra]

8th? instalment of #2357. This one not only makes the debouncer lock-less but also potentially more efficient.

The debouncer associates delayed actions with project files. It works by waiting for a short delay before executing the action. If a new action is registered for this file before the delay is over, the old action is throw away and the delay restarts.

The old implementation does this using async, and a new thread is spawned on every reset.

This PR replaces threads by STM transactions. Associating an action starts an STM transaction that reads the TVar and then sleeps for the delay. Reassociating a new action writes to the TVar causing any long-lived transactions to retry.

STM transactions should be more lightweight than threads but I don't expect this to make a huge difference in practice and wouldn't oppose to reverting the change.

[michaelpj]

I have failed to understand what the code here is dong, so suggestions may be nonsense :D

[michaelpj]

Do we actually need the inner TVar here? What if the async thread just did an STM map lookup to get the value? Wouldn't that have the right properties already?

[pepeiborra]

Not exactly, as the transaction would range over all the contents of the STM Map (or rather, all the contents of the branch that holds this key)

[michaelpj]

Wait, but it already does that because it has to delete the key at the end!

[pepeiborra]

That's no good. The delete needs to happen in a separate transaction. But then there is a tiny race condition window. Hmm

[michaelpj]

Okay, so one option would be to just accept that we're locking a bit of the map, and write some simpler code that just looks up the value in the map.

Another perhaps ugly idea: store a TMVar instead of a TVar in the map. Then the async task can:

1. In a transaction: try to look at (and empty) the TMVar, and then do the action
2. In a transaction: look at the TMVar, if it's empty, then remove it from the map (otherwise someone has put something else in there, so we want to leave it anyway).

That is, we use a TMVar as a one element queue, and the async thread first draws from the queue atomically and executes an action, and then optionally tries to garbage-collect the queue so as to keep the map small.

[michaelpj]

So then there's a race window between 1 and 2, but it's benign, because all that can happen is that someone has put something in the queue again, in which case we can just not garbage-collect it.

[pepeiborra]

Ok, I have gone with a TVar (Maybe ..) and simplified your solution a bit by never deleting from the map

[michaelpj]

That makes me a little nervous - isn't that a space leak?

[pepeiborra]

Technically yes, but a bounded one. Only in projects with infinite files would this be a worry.

[michaelpj]

Worth a comment.

[michaelpj]

(Sorry for the repeated rounds of comments, you keep making it clearer and then I see more things 😅 )

[pepeiborra]

Let's do this in steps and land the lock-less change first, extracted to #2469

[pepeiborra]

Closing as I cannot measure any improvement over the current async debouncer

[michaelpj]

Yeah, I guess you could argue that this version is easier to understand, though?