delta icon indicating copy to clipboard operation
delta copied to clipboard

Cache the Git remote URL to speed up rendering hyperlinks

Open charlievieth opened this issue 1 year ago • 1 comments

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

charlievieth avatar Jan 11 '25 22:01 charlievieth

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.

th1000s avatar Jan 12 '25 21:01 th1000s

@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 avatar Aug 14 '25 01:08 charlievieth

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

charlievieth avatar Sep 05 '25 02:09 charlievieth

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.

dandavison avatar Sep 05 '25 02:09 dandavison

@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 (d5e0565cbfa47acde98d41d8777ace5d1bc4d690). 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

charlievieth avatar Sep 05 '25 03:09 charlievieth

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.

dandavison avatar Sep 05 '25 03:09 dandavison

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 https://github.com/dandavison/delta/pull/2038 to address the clippy complaints.

charlievieth avatar Sep 09 '25 22:09 charlievieth

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.

@dandavison now that https://github.com/dandavison/delta/pull/2038 is merged any chance you can take a look at this PR?

charlievieth avatar Nov 13 '25 00:11 charlievieth

The clippy error reported here also occurs on the main branch.

charlievieth avatar Nov 13 '25 00:11 charlievieth

Thank you for this improvement, and apologies for the delays. I also fixed the new v1.91 (which was released in the meantime) clippy warning.

th1000s avatar Nov 14 '25 09:11 th1000s

@th1000s thank you for the review and for merging this PR!

charlievieth avatar Nov 14 '25 09:11 charlievieth