Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add support for symlinked files and dirs #68

Merged
merged 1 commit into from
Aug 6, 2024

Conversation

jcdickinson
Copy link
Contributor

This adds support for searching through symlinked files and directories, the path is first canonicalized - which will resolve nested symlinks to the real path. Due to the nature of symlinks it's still possible for an infinitely recursive data structure to arise, or for the same canonical file to be repeated. Both cases are covered by keeping track of which canonical files have already been visited.

These changes are required to correctly support some Nix scenarios, specifically installing fonts through home-manager.

@RazrFalcon
Copy link
Owner

This change is very expensive. System fonts loading is up to 50% slower on macOS. I cannot accept it.

We have to ignore duplicated fonts.

Also, I don't know how Nix works, but can we follow symlinks only for files, ignoring directories? Or will it not work?

@jcdickinson
Copy link
Contributor Author

@RazrFalcon Nix uses symlinks very heavily, so it needs full support. I'll think about how to optimize this more.

@jcdickinson jcdickinson force-pushed the symlink-support branch 6 times, most recently from 818df07 to b0423a5 Compare July 30, 2024 23:46
@jcdickinson
Copy link
Contributor Author

jcdickinson commented Jul 30, 2024

@RazrFalcon, I have made the following changes:

  • Always support adding symlinked files. This codepath is avoided if no symlinks are found, so should be zero cost.
  • Allow directory symlink searching to be disabled with the dir-symlink feature flag.
  • So long as a symlink is never encountered, the dir-symlink should be very close to no different.
  • More syscalls are unavoidably involved when encountering a symlinked file or directory, so these will always be more expensive.
  • Add a criterion benchmark.

Here are the results from my machine, your machine will likely/hopefully show almost no difference.

Before

To test: make the second fn canonicalize always return the first value.

cargo run --example find-system-font --release
Loaded 464 font faces in 9ms (19μs per).
Error: 'Times New Roman' not found.
cargo bench
load system fonts       time:   [15.849 µs 15.897 µs 15.947 µs]

Without dir-symlink

cargo run --example find-system-font --release
Loaded 464 font faces in 10ms (23μs per).
cargo bench
load system fonts       time:   [19.768 µs 19.841 µs 19.916 µs]
                        change: [+24.201% +24.807% +25.420%] (p = 0.00 < 0.05)

With dir-symlink

cargo run --example find-system-font --release
Loaded 504 font faces in 13ms (27μs per).
cargo bench
load system fonts       time:   [22.803 µs 22.822 µs 22.842 µs]
                        change: [+14.579% +15.025% +15.461%] (p = 0.00 < 0.05)

@jcdickinson
Copy link
Contributor Author

jcdickinson commented Jul 31, 2024

I just realized that canonicalization is not required when dir symlinks aren't being followed, changes pushed:

vs previous commit

load system fonts       time:   [25.457 µs 25.495 µs 25.536 µs]
                        change: [-20.722% -20.594% -20.454%] (p = 0.00 < 0.05)

vs current code in master

load system fonts       time:   [25.978 µs 25.988 µs 25.998 µs]
                        change: [+8.4845% +8.7590% +9.0271%] (p = 0.00 < 0.05)

@jcdickinson jcdickinson marked this pull request as draft July 31, 2024 05:24
@jcdickinson jcdickinson marked this pull request as ready for review July 31, 2024 05:26
@jcdickinson
Copy link
Contributor Author

Fixed a bug which basically turned the file-only implementation into a noop. Results on my system:

No changes

load system fonts       time:   [24.501 µs 24.517 µs 24.533 µs]
...
Loaded 81 font faces in 2ms (28μs per).

With file symlink support

(and symlinked files). It's faster? I wonder if it's behaving favorably with some kernel cache.

load system fonts       time:   [21.658 µs 21.673 µs 21.689 µs]
                        change: [-11.678% -11.599% -11.512%] (p = 0.00 < 0.05)
...
Loaded 122 font faces in 2ms (24μs per)

With dir support

load system fonts       time:   [25.220 µs 25.236 µs 25.252 µs]
                        change: [+3.1805% +3.4172% +3.6518%] (p = 0.00 < 0.05)
...
Loaded 122 font faces in 3ms (27μs per).

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@drupol
Copy link

drupol commented Jul 31, 2024

Hey, Nix and Typst user here!

I'm looking for that fix too since my Typst build environment based on Nix manage fonts using symlinks, using fontdb 0.18 on the development branch of Typst is simply not working any more. I made a temporary project to show the issue if you're really interested at https://github.com/drupol/typst-font-issue

Looking forward for the next fontdb release, including this fix!

@jcdickinson
Copy link
Contributor Author

Pushed changes to address PR comments. Before:

Loaded 81 font faces in 2ms (28μs per).

After:

Loaded 122 font faces in 3ms (27μs per).

I'm very confused as to why there is no performance difference, even though there are more syscalls and hashing the PathBuf (which I did encounter last night, maybe a 💩 implementation?). Still, no complaints.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@RazrFalcon
Copy link
Owner

using fontdb 0.18 on the development branch of Typst is simply not working any more

Symlinks were never supported.

@drupol
Copy link

drupol commented Aug 1, 2024

using fontdb 0.18 on the development branch of Typst is simply not working any more

Symlinks were never supported.

But it was working before 0.18, right?

@RazrFalcon
Copy link
Owner

Only if Typst was handling them on its own before.

@EpicEricEE
Copy link

It was working before because prior to #65, Path::is_file and Path::is_dir were used, which traversed symlinks automatically. It was then changed to DirEntry::file_type, which doesn't do that anymore.

@RazrFalcon
Copy link
Owner

@EpicEricEE Interesting. I didn't know that.

@jcdickinson jcdickinson force-pushed the symlink-support branch 3 times, most recently from ffe9900 to e5bf6f9 Compare August 6, 2024 03:38
This adds support for searching through symlinked files and directories,
the path is first canonicalized - which will resolve nested symlinks to
the real path. Due to the nature of symlinks it's still possible for an
infinitely recursive data structure to arise, or for the same canonical
file to be repeated. Both cases are covered by keeping track of which
canonical files have already been visited.

These changes are required to correctly support some Nix scenarios,
specifically installing fonts through home-manager.
@jcdickinson
Copy link
Contributor Author

Made some changes to get it to compile on MSRV: https://github.com/jcdickinson/fontdb/actions/runs/10259900356

@RazrFalcon RazrFalcon merged commit 01a2ba4 into RazrFalcon:master Aug 6, 2024
4 checks passed
@RazrFalcon
Copy link
Owner

Looks good now. I don't see any significant performance differences on macOS loading 1396 font faces.

@drupol
Copy link

drupol commented Aug 6, 2024

Looking forward for the new tag! Thanks for merging this!

@RazrFalcon
Copy link
Owner

Published.

@jcdickinson jcdickinson deleted the symlink-support branch August 6, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants