🎬 nodegit 2: The Revenge of nodegit! 😱

[BinaryMuse]

So we have some slowness going on. A lot of this is due to queueing up Git operations and making sure writes get executed alone. This is exacerbated by the fact that we're shelling out for all operations.

We used nodegit in the past, but stopped because it was awful crashy for us. The theory was that this was due to write contention, or the fact that we didn't have/didn't use nodegit's threadsafe mode. This PR implements some of the read-only Git commands using nodegit, resulting in significant improvements at a glance:

command	shell out	nodegit
getCurrentBranch	6ms	0.4ms
getCommit	7ms	0.6ms
getBranches	9ms	1ms
getBlobContents	6ms	0.3ms
getConfig	6ms	0.3ms
getStatusesForChangedFiles	14ms	4ms

And these times are during unit tests, so are a bit artificial.

After using this for a while in a real-world scenario and taking a look at the waterfalls, it definitely seems like it's helping, though certain Git commands are still taking a considerable amount of time.

[smashwilson]

"so looks like using nodegit to get status doesn’t write to the index at all, so we made the conversion and now the check status operation is parallellellizable" - @kuychaco

Even if the speed improvements weren't noticeable, I'd say that this alone ☝️ justifies using nodegit for status. Having to close Atom to do a git rebase to avoid getting index.lock errors is not a great experience.

[BinaryMuse]

Hm, in certain repos we're seeing getStatusesForChangedFiles take upward of a couple hundred milliseconds. All the time is spent in repo.getStatus(). I'm going to do some more testing, but my hunch is that if we can implement all calls to git status with nodegit, that the wins around allowing them to run in parallel and not accidentally trampling command-line actions by writing index.locks all the time will beat out a slightly slower implementation (especially given the other performance wins).

[joshaber]

Heyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy 👋 🚶 📖

So I have a few specific bits of feedback and one more general. It's entirely possible I don't have all the context of this PR, in which case feel free to tell me! 💖

---

Specific first 🎯

1. so looks like using nodegit to get status doesn’t write to the index at all

That's true, but that comes with a downside. git status writes to the index because it updates the index's stat cache. That ensures future status calls are cheaper since git can then check only the files that have changed since the last call, essentially amortizing the cost over time.

This is especially important in repositories with lots of files.

2. libgit2's status implementation mostly works, but there are latent bugs around gitignores. We battled these for a while on Desktop Mac and there are still some open issues. See:

• .gitignore folder negation not working correctly libgit2/libgit2#3989
• status_should_ignore returns true for any path starting with "./" libgit2/libgit2#3979
• Ignore rules are not always respected — depending on ordering of rules and check-in status of other files libgit2/libgit2#3535
• Pattern-based exceptions in .gitignore not working libgit2/libgit2#3224
• Subrepository's gitignore negation rules overrides root repository gitignore (in some cases) libgit2/libgit2#3133

Because of that we specifically steered Desktop Windows away from using libgit2 for status.

3. Having to close Atom to do a git rebase to avoid getting index.lock errors is not a great experience.

(If this line was a throwaway, then feel free to ignore me.)

If the GitHub package is always holding a lock on the index, that seems to indicate a bigger problem than just holding it while getting status.

4. Bringing git interactions in-process can be problematic for 32-bit binaries. The Desktop Windows app ran into this. With big repositories, it's totally possible to run out of address space. @shiftkey is the one most familiar with this pain.

---

More generally 🌏

I wanna make sure we're thinking about this strategically (puts on his mr. manager hat 🎩).

The Atom and Desktop teams met last summer to talk about our shared approach to git, and one of the reasons we deliberately chose not to use libgit2 is because it's unsupported/poorly supported. No one is investing in it. That's still true.

Git interaction is a shared concern across both the Atom and Desktop teams. In the next year we should work to share more of our git code. Branching (lol) our approach to git by using libgit2 moves us away from this goal.

[BinaryMuse]

Thanks for the input, @joshaber!

git status writes to the index because it updates the index's stat cache. That ensures future status calls are cheaper since git can then check only the files that have changed since the last call, essentially amortizing the cost over time.

If the GitHub package is always holding a lock on the index, that seems to indicate a bigger problem than just holding it while getting status.

Right. Although looking at recent waterfalls, the time of git status is pretty consistent. And since it writes an index.lock whenever we run it, which is whenever we detect changes, we can run into the index.lock contention problem (mainly when someone is using a different Git tool or is on the command line doing Git stuff). Perhaps we could relax refreshing if we know we're in the background?

Furthermore, since we group reads and writes and only let reads run in parallel, git status (which we consider a write so we don't run into index.lock contention issues) can't be parallelized when shelling out, and it can be when using nodegit (even though it's slower).

libgit2's status implementation mostly works, but there are latent bugs around gitignores

libgit2 is [...] unsupported/poorly supported. No one is investing in it. That's still true.

Yeah, this is very compelling in general. Do you think it's still safe to use for basic read operations (like looking up commits/remotes/etc) which is what we'd like to use it for? We don't want to use it for any write operations (unless you include status).

Bringing git interactions in-process can be problematic for 32-bit binaries

Super valid concern, although I think the plan is to phase out 32-bit Atom builds completely in the nearish future.

I wanna make sure we're thinking about this strategically

Git interaction is a shared concern across both the Atom and Desktop teams. In the next year we should work to share more of our git code.

I agree in general. My impression was that we landed on this because it looked like the use cases would be very similar and we'd otherwise be duplicating work. However, I'm not 100% sure we should force that if the current strategy isn't meeting the needs of one team or another (though I can certainly see the argument that we can spend more time working together to resolve the issues).

[shiftkey]

And since it writes an index.lock whenever we run it, which is whenever we detect changes, we can run into the index.lock contention problem (mainly when someone is using a different Git tool or is on the command line doing Git stuff).

Semi-related tangent - VSCode made some changes to their Git operations in a recent update and they're causing the exact same issue for us when we're in Desktop:TNG. It really feels like the same behaviour you're describing here with index.lock being created in the background. I've got an open issue to reproduce it and essentially tell them off.

[BinaryMuse]

@shiftkey How are you avoiding that problem in TNG? Just not running status very much?

[shiftkey]

@BinaryMuse we don't do anything when the application is in the background. That behaviour might change in the future, but it's Good Enough For Now™.

[shiftkey]

Bringing git interactions in-process can be problematic for 32-bit binaries. The Desktop Windows app ran into this. With big repositories, it's totally possible to run out of address space. @shiftkey is the one most familiar with this pain.

It's been a while since I've looked into these, but here's some previous anecdotes involving both read and write operations:

• https://github.com/github/Windows/issues/5975 - git_revparse_single issue with UnrealEngine (trying to lookup a commit)
• https://github.com/github/Windows/issues/5815 - various issues noticed after an update - git_revwalk_next (enumerating the history), git_diff_tree_to_index (updating the index as part of making a commit), git_diff_tree_to_tree (retrieving the number of files in a commit), and probably others.

And while dropping support for 32-bit applications is a way to workaround address space limits, what I wanted to call out here was a couple of things:

• loading repositories in-process through nodegit means the memory management of operations on large/complex repositories is something to be mindful of.
• I have no idea how vulnerable nodegit is to memory leaks when pushed to the extremes, but some corner case issues reported on the repository like rss memory grow which the project has 10G large nodegit/nodegit#1159 and Memory leak while iterating over commits and their diffs? nodegit/nodegit#766 are interesting.
• repositories in the wild do some really interesting things, and lots of corner cases have been hard to track down and resolve in the past, because users are not willing (or able) to make the repository available to you to test.

The Atom and Desktop teams met last summer to talk about our shared approach to git, and one of the reasons we deliberately chose not to use libgit2 is because it's unsupported/poorly supported. No one is investing in it. That's still true.

Another footnote to this - when we caught wind of Microsoft moving away from libgit2 in the middle of last year, we sat down with them to understand more of this situation. The discussions there were also very similar to what we've been talking about here, and they were really unhappy with the behaviour differences they stumbled upon between libgit2 and Git for Windows, and they decided to focus on improving Git for Windows. They won't be shipping libgit2 in the upcoming release of Visual Studio 2017, but they continue to use libgit2 for some server-side things (I forget which parts of the company, but I know the Git people at MSFT aren't happy with this).

[smashwilson]

I checked out the source for status in the git source and, if I'm reading this correctly, status will only write to the index if it's able to acquire the lock, but it won't fail if it can't. (Someone mentioned this as a possibility which prompted me to look -- either @shiftkey or @BinaryMuse?) If we can prevent the status subprocess from being able to write to that file somehow, we could have the best of both worlds: the faster status invocation from shelling out and the lack of contention for other tools. I'm not sure offhand what the best, most portable way to accomplish that would be, though - Node's ChildProcess does let you set a uid for the process, so maybe we could do something with that?

If we could find a way to make that work, we could even run status with write access once in a while to take advantage of cache.

Disabling the status call when Atom/Desktop aren't in the foreground is a pretty reasonable workaround, though.

[smashwilson]

The Atom and Desktop teams met last summer to talk about our shared approach to git, and one of the reasons we deliberately chose not to use libgit2 is because it's unsupported/poorly supported. No one is investing in it. That's still true.

This makes me sad, I really like the idea of libgit2 😢 As much as I like the UNIX-y philosophy of using stdin and stdout as a universal interprocess communication channel, shelling out will always feel "heavy" to me.

I hadn't thought about address space limitations, though, and being guaranteed to reclaim memory from repository operations is a pretty big upside, especially as RAM-hungry as Electron apps already have a reputation of being.

Another alternative @BinaryMuse brought up a while ago that I liked was to implement selected git functionality directly with Node I/O, especially for the less involved and more stable operations like "reading blobs" or "dereferencing HEAD." We could even add tests to git-kitchen-sink to verify that our implementations stay consistent with the bundled git binaries. That'd lessen the burden on our command queue and drop the latency on those operations (although the easy-to-port ones aren't our most expensive, of course). With the performance measurement tooling we'll have some data to back up the most worthwhile candidates to port, too 🔬

[BinaryMuse]

Okay, we've chatted with multiple people, and we've decided to put the "no" in "nodegit." We may experiment with doing certain operations (like reading blobs, if they exist) with raw JS, but for now we'll do with the improvements we've made to queue parallelization and to ModelObserver.