fix: redraw events are processed in order

[ollien]

REVIEW NOTE: This is almost certainly going to be easier to review if you ignore whitespace when viewing the diff.

This is a bit of an untested prototype. I'm not 100% what it fixes or breaks, but it comes back to a comment I left on another issue. The TL;DR is that neovim actually assumes we are processing redraw events in order, but we don't guarantee that, due to the way the eventBus is set up (see the linked comment for more details). My suspicion is that this will fix a couple of those "random" bugs we see here and there, but I can't prove that.

There's a couple of things that seem suspect to me immediately.

1. Cursor events will fire processCursorMoved after each item in the redraw batch, rather than after all the events have completed. This may or may not be a problem, but it may be prudent to build some way to flush those updates to the UI only when the batch finishes.
2. The statusline manager checks if mouse events exist within the batch, but I've commented that check out for now. It shouldn't be hard to build some kind of proxy for it, though.

Thoughts are welcome!

[ollien]

Looking back, it seems that (2) in my above list was a hack to fix this issue #372. The fork of that extension doesn't work anymore, and I couldn't produce the case with the original extension (more accurately, I couldn't get the binding for <leader>/ to actually produce jump-points), so I'm inclined to just fix this by removing that hack and avoid adding complexity to solve it

[xiyaowong]

I tried this before, but I don't quite remember why I didn't continue. It could be due to performance issues, or sometimes certain modules would conflict. Not sure.

Theoretically, this PR does address certain issues, although these issues are currently also quite rare.

[justinmk]

I don't see how this could cause any performance impact. And it's more correct. Using map() to process a redraw batch in arbitrary order is completely wrong.

[xiyaowong]

I don't see how this could cause any performance impact.

More "fire"

Using map() to process a redraw batch in arbitrary order is completely wrong.

The issue here is not with the "map" itself; the "map" here merely encapsulates the data. It's entirely possible to iterate through the data after the mapping process.

[justinmk]

More "fire"

ok, then should the handler for eventBus.fire("redraw", ...) be changed? i.e. send a batch to eventBus.fire("redraw", eventData), as before. Then whatever handles that, should respect ordering, if it doesn't already.

[xiyaowong]

Some modules need data initialization for redraw handling. Currently, each redraw requires initialization every time. Additionally, this approach can't ensure 100% sequential event execution. We lack a lock. Sequential execution becomes synchronous, potentially causing lag, especially with the highlight manager due to VSCode's processing speed.

[ollien]

Thanks for the look guys. Nice to know I'm not completely off-base. I haven't gotten through what I need to in order to fix my cursor manager concerns (and the highlight manager too, probably, for similar reasons). My thought is I can emit some kind of event once all the "redraw" handlers finish (a "redraw-done" event of some kind), but I'm still exploring the best way to do that.

Just to circle back to some of the above conversation:

More "fire"

I don't think "fire" per-se is a problem. Under the covers, "fire" is mostly just a for loop to broadcast to each listener (there are some asterisks on that), so we're effectively just "moving" the redraw handler calls around.

Some modules need data initialization for redraw handling. Currently, each redraw requires initialization every time.

Can you think of any examples that concern you? A quick look doesn't show very much work going on per event.

Additionally, this approach can't ensure 100% sequential event execution. We lack a lock. Sequential execution becomes synchronous, potentially causing lag, especially with the highlight manager due to VSCode's processing speed.

It's true, we can't have 100% sequential event execution with this approach, since, as you point out, there are some modules that require asynchronous execution. This is definitely going to be a lot closer though, given JS is effectively single-threaded, and no other call will run until the handler returns (or does an await etc, but it's technically still returning :)).

[xiyaowong]

I cannot provide specific examples either. When I previously attempted to distribute events one by one, some strange issues did arise. Perhaps with ongoing fixes, this potential factor may have disappeared. In any case, I just brought it up to indicate which aspects need attention.

[ollien]

Highlights were a fun problem to solve, given the use of async code in there. I stole a concept from Go, WaitGroups, to ensure that flush events wait for the redraws to complete :)

My cursory attempts at usage seem good, but I'm going to try and run this for a bit to see how it goes.

[justinmk]

Nice re-use. This makes me notice that PendingUpdates has set-like behavior, does that mean the addForceUpdate method has a misleading name? Because it's not appending something, but rather setting a key, which may overwrite the previous entry.

[ollien]

Aha! So, yes and no. It has set like behavior in that for each key you add, it only stages that resource as a single entry (check out this test). If we were to be using conditional updates, this would mean that multiple calls to addConditionalUpdate would have one key, with multiple conditions. So no, nothing is overwritten.

As for the name, yeah, I felt weird writing addForceUpdate here, since there's no contrasting "conditional" usage. I welcome an alternate name though!

[justinmk]

Semaphores have the same problem (one of many). Seems like this is reinventing semaphores, but nowhere does it mention that.

Assuming this is actually needed, maybe we should take a dependency on a small, high-quality library instead of starting from scratch. Avoiding dependencies is very important, but if this is actually needed, then it is likely to grow features and logic, and will end up reinventing a well-known data structure. This doesn't seem like novelty that benefits us.

[ollien]

This is not quite a semaphore, or at least, not classical ones (maybe there is a variety I'm not aware of?). Semaphores block access to a critical section by allowing a certain number of resources through, and waiting until a slot is available. This, however, mandates that all tasks complete before the promise resolves. Consider the following sequence

wg.add() // task 1
await wg.promise // somewhere else
wg.add() // task 2
wg.done() // task 1
wg.add() // task 3
wg.done() // task 3
wg.done() // task 2
// only here does the promise resolve

As for a dependency, yes, absolutely. I actually was looking at https://www.npmjs.com/package/@jpwilliams/waitgroup, but figured this was so small that re-implementation might be worth it (and I know this project has tried to avoid more dependencies in the past). That said, I agree with you regarding it being well tested, and don't mind pulling it in.

[justinmk]

https://www.npmjs.com/package/@jpwilliams/waitgroup has no dependencies and has some more docs + references to Golang analogs, so +1

[theol0403]

Great PR! Can you update CONTRIBUTING.md with an explanation of this PR? Alternatively, write a doc block in the code explaining how the processing logic/async timing works.

[ollien]

@theol0403 Absolutely! I'll prefer to do it in line, if that's ok with you. Easy for lots of things to get missed in a stray markdown document.

[theol0403]

Bug report: the cursor style is incorrect on extension startup

[ollien]

Oh interesting. I think this may have uncovered a subtle bug as a result of event ordering. It seems like mode_info_set is sent after mode_change, so when processing the mode_change, we don't have information about how to render the cursor. A quick fix is to simply call updateCursorStyle every time we get a flush event, but that's probably not the most robust. Will have to think on this a bit

[ollien]

Nope, I just made a silly mistake. I shouldn't be going over the list of events backwards 😅. The splice in main_controller tripped me up

[xiyaowong]

I think "shouldUpdate" is clearer than "update".

Suggested change

	for (const [grid, update] of this.pendingGridUpdates.entries()) {
	for (const [grid, shouldUpdate] of this.pendingGridUpdates.entries()) {

[ollien]

update is a bit clearer in this case, IMO, because there is an actual mutation performed (the update function calls this.highlightProvider.processHLCellsEvent). That said, if you feel strongly I'll change it

[xiyaowong]

Used for two days, working fine.