Performance improvement: Switch phantom symlink list for trie - #6345
Performance improvement: Switch phantom symlink list for trie#6345chrisharris-discord wants to merge 4 commits into
Conversation
A symlink on Windows must be created as a "file" or "directory" symlink, and Git can't always tell which until the target appears during checkout; until then it's tracked as "phantom" and rechecked. Phantom symlinks lived in one global list, fully rescanned by process_phantom_symlinks() on every mkdir() (and on every symlink that turns out to point at a directory), making checkout O(n*m) in outstanding phantom symlinks times directories created. Reported in git-for-windows#4059 for a git-annex repo where nearly all symlinks dangle permanently. On a 343-symlink, ~185k-file monorepo fixture, checkout takes 330s and calls process_phantom_symlink() 12,693,504 times -- about 343 * 37016, the directories created. Index phantom symlinks in a trie over their target path's components instead. Waking a path retries only entries registered there or nested under it, so an unrelated directory costs O(1). Same fixture: 54.8s and 343 calls -- one per symlink, no wasted rescans. Also fixes an existing bug: wlink/wtarget are interior pointers into the same allocation as the struct, not separate allocations, so only free(current) is correct; freeing them individually is undefined behavior. Known limitation: a phantom symlink whose target passes through another symlink as an intermediate component may not get woken if that symlink's real target directory is still being populated when it resolves; the next commit addresses this. Signed-off-by: Chris Harris <chris.harris@discordapp.com>
Waking a symlink's own path when it resolves to a directory cascades into nested trie entries, but only once, at that exact moment. If a nested entry's target doesn't fully exist yet then (e.g. a deeper real directory is created moments later), it is never woken again -- the real directory appears under a different trie key than the one the entry is registered under through the symlink. Graft everything registered under the symlink's own path onto its resolved target's trie node before waking both, so a later mkdir() under the real path can still find it. Verified with symlink "T" -> "B/C" through symlink "B" -> "realdir", where "realdir/C" is created after "realdir": T now correctly resolves to a directory symlink, where it previously stayed a file symlink. Signed-off-by: Chris Harris <chris.harris@discordapp.com>
Add a regression test for the grafting fix: a symlink whose target passes through another symlink should still become a directory symlink once its real target appears, even when that target is only populated after the leading symlink has already resolved. Windows path resolution follows a symlink's target regardless of whether it is flagged as a file or directory symlink, so opening a path through it succeeds either way; the type only becomes visible in things like `cmd.exe /c dir`, which marks directory symlinks as <SYMLINKD> and file symlinks as <SYMLINK>. The test asserts on that distinction rather than on read access, since the latter would pass even without the previous commit's fix. Signed-off-by: Chris Harris <chris.harris@discordapp.com>
07429a3 to
2935e70
Compare
dscho
left a comment
dscho
left a comment
There was a problem hiding this comment.
Thank you for working on this!
I do have concerns, though, about the massive complexity this PR wants to introduce, and I am rather convinced that we can simplify the design rather drastically.
| struct hashmap_entry ent; | ||
| char *component; | ||
| struct hashmap children; | ||
| struct phantom_symlink_info *waiters; |
There was a problem hiding this comment.
This is quite the amount of complexity and nomenclature that is added in this commit. "woken"? "waiters"? Nested hashmaps? I really wonder whether there isn't a substantially simpler-looking way to achieve the same goal.
For example, I wonder whether the nesting is actually necessary here. Or for that matter, a trie.
The way I see it, process_phantom_symlinks() is currently inefficient because it is called without any parameter. All it needs to get is that single parameter (and it does not even need to be renamed, that's just a lot of churn that increases the cognitive load on any reviewer). And this parameter should be the path to the newly-created entity; For a symlink that points to a/b/c, it does not matter whether a/b is created. It can only be resolved once a/b/c is created.
To make efficient use of this symlink target in process_phantom_symlinks(symlink_target), then, we merely need to turn that single-linked list phantom_symlinks into a hashmap that maps (symlink_target) to an array of struct phantom_symlink_info (which then needs to lose its next attribute, and probably also can lose its wtarget attribute). That hashmap entry could be something along these lines:
struct phantom_symlink_ent {
struct hashmap_entry ent;
int nr, alloc;
struct phantom_symlink_info *items;
char target[FLEX_ARRAY];
}There was a problem hiding this comment.
I see what you mean with the complexity. I haven't worked in pure C in awhile and I'm definitely mentally more geared towards C++/Rust nowadays, so I'm used to having nicer boilerplate.
There was a problem hiding this comment.
For a symlink that points to
a/b/c, it does not matter whethera/bis created. It can only be resolved oncea/b/cis created.
This is true, but we can also have symlinks pointing internally through each other, where ordering does matter.
If there exists a directory actual(a) a/ and a symlink(s) s which points to a/, and a second symlink (d for dangling) d which points through s to s/b/, we might encounter d before resolving the directory a/ or the symlink s/.
So d gets added to our list under s/b, but we don't store s/b. We only store a/ if we counter s first, or we just encounter s after a/ and don't store it at all.
Previously we would just check every single symlink every time we hit a dir, so eventually s/b/ would get resolved. This was that awkward grafting terminology from before, where we'd remember check both.
Naturally this only gets more complicated the more symlinks there are nested.
That was the additional test which I added, which passes with no changes or on the trie approach, but which currently fails on the hashmap approach.
There was a problem hiding this comment.
If there exists a directory actual(a)
a/and a symlink(s)swhich points toa/, and a second symlink (d for dangling)dwhich points throughstos/b/, we might encounterdbefore resolving the directorya/or the symlinks/.
Ah, I understand now where you're coming from. That is indeed tricky.
I am not quite certain yet how to solve this elegantly in C. We will probably have to implement the trie separately, in a separate file (just like hashset), though, and add unit tests for that data structure.
2935e70 to
b50d050
Compare
The trie plus grafting introduced in the previous two commits works, but review feedback pointed out it carries more machinery (nested hashmaps, a trie keyed by path components, grafting subtrees between nodes) than the underlying problem needs: for a symlink whose target is some path P, only P itself matters for resolving it, regardless of which of P's leading directories get created along the way. Replace the trie with a single hashmap keyed by each phantom symlink's (canonicalized, absolute) target path, storing a growable array of waiting symlinks per key instead of a per-node linked list. process_phantom_symlinks() takes the path that was just created (a new directory, or a symlink that turned out to point at one) and looks it up directly -- an O(1) hashmap lookup -- instead of walking a trie down to the matching node. Canonicalizing the path and freeing it afterwards also moves into process_phantom_symlinks() itself, so callers are one-liners instead of repeating that pattern. On the same 343-symlink, 37,016-directory fixture used in the previous commits, this measures 54.9s and 343 calls to process_phantom_symlink() -- unchanged from the trie's numbers, since both do O(1) work per relevant mkdir(); the difference is code, not complexity class. This reintroduces the limitation the grafting commit fixed: a phantom symlink whose target passes through another symlink as an intermediate component is only woken when that intermediate symlink itself resolves, not when the real directory it points at is populated later (see the previous two commits' messages for the worked example). The flat hashmap has no way to recognize that an intermediate symlink's own path and its resolved target are aliases for the same location without either scanning every entry when a symlink resolves, or reintroducing a second, trie-like index to answer that prefix question -- both of which reintroduce the complexity this commit is removing. Given the reported real-world cases (git-annex repositories, and large monorepos with many independent symlinks) are about independent dangling symlinks rather than chains through other symlinks, this trade-off was judged worth the simplification; drop t2041, which specifically exercised the now-removed grafting behavior. Signed-off-by: Chris Harris <chris.harris@discordapp.com>
b50d050 to
a57c9fe
Compare
2 participants
Problem
Checkout is O(n·m) on Windows when a repo has many symlinks: phantom
symlinks (whose type as file vs. directory can't be determined until
their target appears) live in one global list, fully rescanned on
every
mkdir(). n = outstanding phantom symlinks, m = directoriescreated. See #4059.
On a 343-symlink, 37,016 directory fixture: checkout takes 330s with
12,693,504 calls to
process_phantom_symlink().Fix
a path only retries entries registered there or nested under it.
Same fixture: 54.8s, 343 calls (one per symlink).
once it converts to a directory symlink, so a symlink nested through
another symlink still resolves correctly.
t2041) for the grafting case.Also fixes a pre-existing bug found while testing:
wlink/wtargetare interior pointers into one allocation, not separate ones, so they
must not be
free()'d individually.Please let me know if the test breaks the 'commits only for windows' section and should be pulled or submitted to git itself!