watcher refactor

[Byron]

This PR attemps to minimize the watcher implementation to only the needed complexity.

Based on the analysis in #3447.

Tasks

• (warmup): try_from() implementation becomes a constructor method.
• See if consuming run() is accidental (it was)
• reorganize dispatcher to work without extra objects and without cancellation token (channels are all that's needed for producers to shut down when consumers shut down, and the dispatcher is a producer)
• See if each Handler needs to be managed - are they used and if so, how? Those who aren't shouldn't be managed.
  • It seems they are only managed because they check if they are managed - a catch22?
  • Careful: this could break the app, but if it does, hopefully thoroughly so it's obvious.
• Remove Clone from Watcher by allowing communication to it using a channel
  • That way, the channel can be Clone, but the watcher doesn't have to be.
    This is a precondition for removing all the interior Send+Sync and locking protections,
    while the Watcher will have to watch in the background and thus be in a loop.
• try to implement each Handler as function or method.
• remove the event loop by making relevant calls directly (as much as possible)
  • remove last pure suffixed function and fixup tests to allow dissolve the last remaining private events
  • fix tests - needs tauri support, which works with mocks
• fix as many remaining TODO(ST) as possible (only 3 remaining, 2 of which I might fix here)
• make events::Event just Event
• check usage (also after changing of .gitignore at app runtime) of ignore-filter to see if it can be removed/improved to be faster
• try to parallelise delta computation
  • Performance test was git clone https://github.com/BurntSushi/ripgrep, then gco @~200 -- crates/. After each run, the worktree was reset to @.
  • It's definitely working with granularity of a single file, and it's way faster than the single-threaded version
• validate that it is not slower than the application on master
  • It's not necessarily faster than the ultra-parallel auto-parallelizing version of the watcher in master. Something I don't understand is that a single file here costs 12s runtime on a single thread, yet master is done way earlier than that (or at least, logging stops earlier). It's like it is not seeing all the file or something else is going on that causes it to skip computations maybe?
• Create issues for application bugs after re-validating them
• Only watch one project at a time

Notes for the Reviewer

• The delta computation consumes the most CPU, and if it's truly unused, I think it should be removed from the handler at least.
• I removed the rate-limited, as it never kicked in during my testing due to the handling of multiple paths at once. This was an issue to me as well as simply not calculating something would have meant that sometimes, it's not arriving at the correct result.
• I am pretty sure that I never got to test the fetch and push GB repo functionality, maybe much more. Better test everything that is triggered by watchers.
• LOG_LEVEL=info should probably be preferred when testing the performance of git checkout @~100 -- src/ or similar as it will still emit a lot of debug messages that are buried deeply in the core part. This makes it seem like a lot is happening, even though that's not the case anymore in comparison to the before-times.
• I adjusted the log level of a few traces in core to allow launching the application in DEBUG mode without being spammed with low-level information. Instead, DEBUG is now used for select information, and everything else was demoted to the TRACE level. It's probably the way things go, what's useful in DEBUG one day is better as TRACE in another as the focus changes.

Application Bugs 📋

These bugs were encountered during testing.

• Adding a file is registered, but removing it often is not. Creating and removing empty files often doesn't register the removal (MacOS) #3511
• Worktrees probably aren't supported - judging by the assumption in file_monitor.rs, .git has to be a directory which isn't the case for worktrees.
  • still needs to be formally reproduced, but "Add new project" in a worktree fails due to not being able to read the objects directory. #3062 already talks about this. and what's mentioned here is definitely related.
  • possible fix - detect this case and install a separate notification stream on the extra git directory.
• Trying to open the linux kernel freezes for a long time, probably because it wants to download gravatar images of 30k or so contributors. See Adding the Linux Kernel as project takes more than an hour to complete #3512 .

Out of Scope but TODO

• git::credential::Helpers::from_path() is used in tests, but depends on the actual HOME environment variables, isn't isolated
  • Maybe check other from_path() implementations as well.

My Notes

Definitely not a mandatory read to review this PR.

• There seem to be redundant calls to is_path_ignored() in many places, given that the dispatcher filter already avoids ignored files (and can do so much more efficiently, but we must assure that it always gets most recent information .gitignore files as the watcher is long-running).
• In theory, since it's going to be communicating with channels to the outside, there should be no fear of parallelism at all, so all Handlers should have no need. One can hope they are isolated which they are probably not as they are all managed in tauri. This should probably be rolled back as much as possible.
• Send+Sync + shared state and Clone are poison, and a way to feel unsafe even without the keyword. In such an environment, one has no garantuees anymore, so one will want all code not to be dealing with that.
  • This is probably the entrypoint to unravelling Send+Sync in core as well, even though it might also be a good hint at how these core instances can be used. My feeling is that many will need a tauri-specific wrapper, so that core can be pure.
• There is a large amount of boilerplate just to deal with all the blanket 'protections' needed for Send+Sync, many handlers are just a couple of lines. One handler is just sending an event, effectively.
• The current event queue system is actually very cleverly multi-threading everything. Each event coming in is processed in its own (or pooled) thread, and as each even can spawn new events, these will equally be queued and run in parallel. This is why everything is Send + Sync in the watcher.
• Seeing the .git/GB_Flush file, I wonder if this file-based notification system is truly necessary, or if that could be kept in tauri. Maybe the plan is to support multiple windows to be open on the same repository (rather than prevent it), which probably would make this file-based approach necessary, and maybe even the most simple solution if filesystem events can be assumed to be reliable.
• There definitely is some business logic in the watcher that probably wants to move to core at some point,.
• There is generally a lot of rather generic string-only context() calls and it feels these are 'just there to be there' even though the callee probably could deliver decent error messages from the beginning (and maybe already does so).
• Tests are definitely easier to write with the Events system as side-effects are nicely decoupled via returned events. Now that everything is coupled again, it becomes clear how much state is required for certain functions, making the initial test setup more involved.

[Qix-]

Is there a reason for crossbeam channels vs using tokio's channels?

[Byron]

Actually, tokio channels might be usable here and I didn't try. It would certainly be preferable if it worked.

[Qix-]

Let's try tokio then, lmk if it doesn't work for some reason. Just trying to reduce our number of dependencies.

[Byron]

I did take a look and realised that tokio::sync::broadcast is indeed able to provide the multi-multi case. Probably last time I didn't really look there though since it requires async, whereas this code is all non-async.

crossbeam-channel was previously used in the dependency graph by 5 other dependencies and isn't adding to the compile time.

Do you still think crossbeam-channel should be replaced with tokio::sync::broadcast? If so, I'd probably go with block_on where needed as this implementation relies on std::thread::scope(). Turning it into async-proper for parallelisation would be more effort.

[Qix-]

That's probably fine, we can move more toward async over time.

Also TIL about std::thread::scope() - neat!

[Byron]

Thanks for starting the review, @Qix- !

I recommend to gh pr checkout 3471 this PR and push fixes and changes directly into it. For bigger questions, I will answer everything that might be coming up here. Thanks again.

PS: @mtsgrd I replied on top of your replies in some comments as what I saw was out of date, not because I thought I had to add to them.

[Byron]

Thanks for the review! I think I have addressed all comments thus far, please let me know if anything else comes up.

[Qix-]

Thanks!