Performance improvement: Switch phantom symlink list for trie - #6345
Open
chrisharris-discord wants to merge 7 commits into
Open
Performance improvement: Switch phantom symlink list for trie#6345chrisharris-discord wants to merge 7 commits into
chrisharris-discord wants to merge 7 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>
chrisharris-discord
force-pushed
the
chrisharris/mingw-phantom-symlink-trie
branch
from
July 21, 2026 21:39
07429a3 to
2935e70
Compare
dscho
reviewed
Jul 30, 2026
dscho
left a comment
dscho
left a comment
Member
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.
chrisharris-discord
force-pushed
the
chrisharris/mingw-phantom-symlink-trie
branch
from
July 30, 2026 18:28
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>
chrisharris-discord
force-pushed
the
chrisharris/mingw-phantom-symlink-trie
branch
from
July 30, 2026 18:52
b50d050 to
a57c9fe
Compare
Add a small data structure that maps file system paths to lists of intrusive entries, keyed componentwise, so that -- unlike a flat hashmap keyed by whole paths -- prefix questions can be answered: all entries registered at or below a path can be removed in one operation (path_trie_drain()) or moved onto another path (path_trie_move()). Both operations are needed when a path turns out to be an alias for another path, as with symbolic links on Windows: a symlink whose type cannot be determined yet may have a target that passes through another such symlink, and once the latter resolves, everything registered through it must be findable via the real path it points at. The trie records each entry's current registration path in the entry itself, and path_trie_move() rewrites it, so a drained entry can always be re-registered where it was last filed. Component comparison is optionally case-insensitive, chosen at init time. This will be used by the Windows-specific phantom symlink tracking in compat/mingw.c in the next commit; the data structure itself is platform-independent and comes with unit tests. Signed-off-by: Chris Harris <chris.harris@discordapp.com>
The flat hashmap introduced two commits ago wakes only the phantom symlinks whose target is exactly the path that was just created. That misses targets that pass through another symlink: given a symlink "leading" -> "realdir" and a symlink "nested" -> "leading/sub", "nested" is registered under "leading/sub", but the directory that eventually appears is created under "realdir/sub" -- a different key aliasing the same location -- so "nested" is never woken and stays a file symlink. The old unconditional full-list rescan caught this case by brute force. Switch the tracking to the path trie added in the previous commit. Whenever a symlink resolves to a directory, everything registered at or below its own path is moved (path_trie_move()) to the corresponding path under its target, so later mkdir()s under the real path find those entries; the trie rewrites each moved entry's registration key, so an entry that still cannot resolve is re-filed where a future wake will look for it. Waking is a simple worklist: drain everything at or below the created path, probe each entry, and queue the target of every symlink that resolved to a directory, since other phantom symlinks may point through it. Re-add the t2041 regression test for exactly this chained-symlink scenario (it passes with the old full-list rescan and with this trie, and fails with the flat hashmap). On the 343-symlink, 37,016-directory fixture from the earlier commits, checkout takes 52.2s -- unchanged from the flat hashmap (54.9s) within noise, and still down from 330s before this series. Signed-off-by: Chris Harris <chris.harris@discordapp.com>
The helper only consults the trie's icase flag, not the trie itself; narrow the parameter to say so. Signed-off-by: Chris Harris <chris.harris@discordapp.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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!