Skip to content

Conversation

@charlievieth
Copy link
Contributor

@charlievieth charlievieth commented Jan 11, 2025

This commit speeds up the rendering of hyperlinks by ~47x by caching the Git repos remote URL instead of fetching it each time a hyperlink is rendered.

Fixes #1939

Note

I am very new to Rust so I am very open to feedback and completely understand if you want to rewrite or abandon this entire PR (especially if for some reason the remote URL should not be cached). Additionally, I think the GitConfig should be responsible for storing the cached remote, but struggled to make that work due to borrowing, lifetimes, mutability, etc.

Benchmarks

The below benchmarks were ran in the root of Linux v6.12. We use head -c $((128 * 1024 * 1024)) to limit the amount of Git log data since otherwise benchmarking delta before this change takes a very long time.

TLDR: This change makes processing 128Mib of Git log output take 3.7 seconds instead of 176.1 seconds.

Without git remote caching

$ DELTA_PAGER='bash -c "time head -c $((128 * 1024 * 1024)) | pv --stats --output=/dev/null"' \
    git -c 'pager.log=delta --hyperlinks' log
 128MiB 0:02:56 [ 744KiB/s]
rate min/avg/max/mdev = 654979.457/764202.554/1108112.678/35667.893 B/s

real	2m56.063s
user	0m0.736s
sys	0m5.530s

With git remote caching (this change)

$ PATH="{PATH_TO_NEW_DELTA}:${PATH}"
$ DELTA_PAGER='bash -c "time head -c $((128 * 1024 * 1024)) | pv --stats --output=/dev/null"' \
    git -c 'pager.log=delta --hyperlinks' log
 128MiB 0:00:03 [34.6MiB/s]
rate min/avg/max/mdev = 35979500.033/36287504.626/36646720.334/263641.477 B/s

real	0m3.700s
user	0m0.205s
sys	0m1.690s

@th1000s
Copy link
Collaborator

th1000s commented Jan 12, 2025

Thanks for reporting this!

Caching is fine, the slowdown is from libgit2 being overly correct and assuming that the remote url could change while the program is running.
The PR can indeed be made simpler by using a RefCell to add interior mutability to the GitConfig, i.e. cached_remote: RefCell<Option<Option<String>>> with an not-yet-cached option layer, and another for the result of .find_remote(..).ok() - or maybe a custum enum is clearer.

@charlievieth charlievieth force-pushed the cev/hyperlink branch 2 times, most recently from aeb4fca to c184e5f Compare August 14, 2025 01:53
@charlievieth
Copy link
Contributor Author

@th1000s Took a while until I had some spare cycles to look at this again, but following your guidance I was able to use a OnceCell to embed the GitRemoteRepo struct into the GitConfig struct. Thank you again for your suggestions and would love another review if you have the time.

@charlievieth
Copy link
Contributor Author

@dandavison 1) Thank you for delta - it's incredible 2) Any chance you could enable the tests?

@dandavison
Copy link
Owner

Hi @charlievieth -- tests are running! I'm sorry that I personally am finding so little time to work on delta at the moment, but thank you very much for your work here.

@charlievieth
Copy link
Contributor Author

charlievieth commented Sep 5, 2025

@dandavison Thank you and I completely understand (I've only recently had the cycles to address my own OSS commits and projects).

The clippy failure that I get on this branch/PR are identical to the one I get when running make lint (aka cargo clippy) against the main branch (d5e0565). I'm fairly new to Rust so happy to look into the failure myself and include it into this branch or create a preceding PR (to merge before this one) that addresses the issue.

For context my rust toolchain is:

  • rustc 1.89.0 (29483883e 2025-08-04) (Homebrew)
  • clippy 0.1.89

Script used to check the delta of cargo clippy

#!/usr/bin/env bash

set -euo pipefail

TMP=$(mktemp -d)
TMP_MAIN="${TMP}/clippy.main.txt"
TMP_HYPER="${TMP}/clippy.hyperlink.txt"

if ! git remote -v | grep -q '^charlievieth'; then
    git remote add charlievieth https://github.com/charlievieth/delta.git
fi
git fetch --all

git checkout main
cargo clippy  &> "${TMP_MAIN}" || true

git checkout cev/hyperlink
cargo clippy  &> "${TMP_HYPER}" || true

(
    cd "${TMP}"
    git diff --no-index "$(basename "${TMP_MAIN}")" "$(basename "${TMP_HYPER}")"
)

Output (nothing applicable AFAIK):

diff --git a/clippy.main.txt b/clippy.hyperlink.txt
index 65e5421..71ee65e 100644
--- a/clippy.main.txt
+++ b/clippy.hyperlink.txt
@@ -71,4 +71,4 @@ help: use `'_` for type paths
     |                                                                              ++++

 warning: `git-delta` (bin "delta") generated 5 warnings
-    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.68s
+    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.70s

@dandavison
Copy link
Owner

If you'd like to deal with the clippy complaints then that would be great! I think the best way would be to open a PR for the clippy-related fixes against main and then rebase this on that.

@charlievieth
Copy link
Contributor Author

If you'd like to deal with the clippy complaints then that would be great! I think the best way would be to open a PR for the clippy-related fixes against main and then rebase this on that.

Created #2038 to address the clippy complaints.

Fix Clippy complaints from running `cargo clippy` and `cargo test`.
This commit speeds up the rendering of hyperlinks by ~55x by caching the
Git repo's remote URL instead of fetching it each time a hyperlink is
rendered.

Fixes dandavison#1939
This commit changes the GitConfig struct to cache the GitRemoteRepo,
which speeds up the rendering of hyperlinks by ~55x.

Fixes dandavison#1939
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.

🐛 Hyperlinks make delta 55x slower when used as the pager for git log

3 participants