We ran into a stack overflow in `merge_trees()` at Google due to
limited stack space and large stack frames caused by async code. This
patch fixes that by making `merge_trees()` non-recursive. Since I was
rewriting the algortithm anyway, I also made it concurrent, addressing
a TODO.
I'll make RevsetGraphWalk eliminate transitive edges from intermediate edges to
mitigate out-of-memory issue. This means that edges_from*() functions will call
remove_transitive_edges(). Maybe we could rewrite the whole process to not use
machine stack, but doing that would be complicated. The process wouldn't be
trivially-deterministic on where the look-ahead can terminate either.
The performance problem introduced by this patch will be fixed by later patches.
In small repo:
```
% hyperfine --sort command --warmup 3 --runs 10 -L bin jj-1,jj-2,jj-3,jj-4,jj-5 \
'target/release-with-debug/{bin} -R ~/mirrors/git --ignore-working-copy log -r "tags()"'
Benchmark 1: target/release-with-debug/jj-1 -R ~/mirrors/git --ignore-working-copy log -r "tags()"
Time (mean ± σ): 1.511 s ± 0.091 s [User: 1.296 s, System: 0.214 s]
Range (min … max): 1.432 s … 1.674 s 10 runs
Benchmark 2: target/release-with-debug/jj-2 -R ~/mirrors/git --ignore-working-copy log -r "tags()"
Time (mean ± σ): 2.467 s ± 0.142 s [User: 2.271 s, System: 0.196 s]
Range (min … max): 2.246 s … 2.588 s 10 runs
Relative speed comparison
1.32 ± 0.10 target/release-with-debug/jj-1 -R ~/mirrors/git --ignore-working-copy log -r "tags()"
2.16 ± 0.16 target/release-with-debug/jj-2 -R ~/mirrors/git --ignore-working-copy log -r "tags()"
```
In mid-size repo:
```
% hyperfine --sort command --warmup 3 --runs 10 -L bin jj-1,jj-2,jj-3,jj-4,jj-5 \
'target/release-with-debug/{bin} -R ~/mirrors/linux --ignore-working-copy log -r "tags(v5)"'
Benchmark 1: target/release-with-debug/jj-1 -R ~/mirrors/linux --ignore-working-copy log -r "tags(v5)"
Time (mean ± σ): 937.7 ms ± 68.8 ms [User: 672.1 ms, System: 265.4 ms]
Range (min … max): 868.3 ms … 1025.4 ms 10 runs
Benchmark 2: target/release-with-debug/jj-2 -R ~/mirrors/linux --ignore-working-copy log -r "tags(v5)"
Time (mean ± σ): 1.185 s ± 0.066 s [User: 0.920 s, System: 0.265 s]
Range (min … max): 1.132 s … 1.310 s 10 runs
Relative speed comparison
1.07 ± 0.11 target/release-with-debug/jj-1 -R ~/mirrors/linux --ignore-working-copy log -r "tags(v5)"
1.35 ± 0.12 target/release-with-debug/jj-2 -R ~/mirrors/linux --ignore-working-copy log -r "tags(v5)"
```
When no progress has been made, indicate that as 0.0 progress rather
than dividing by zero. Avoids in particular a display issue where the
progress bar indicates NaN% progress when fetching from slow remotes.
Fixes#7155
Change-Id: I6a6a6964d6572d1c98b5fa25285d26c07ee27a40
This allows evaluating ancestors/ranges involving filters significantly
faster. For instance, naively evaluating `::mine()` requires reading
every commit in the repo to check `mine()` on each commit, then finding
the ancestors of that set. This optimization rewrites `::mine()` to
`::heads(mine())`, since `heads(mine())` can be evaluated more
efficiently by only reading commits until the first successful match.
If someone is unaware of how revsets are implemented, this case can come
up pretty easily, such as by including `~mine()` in `immutable_heads()`
to make other people's commits immutable. In my local `jj` repo, this
optimization reduces the runtime of `jj log` with `~mine()` in
`immutable_heads()` from about 800ms down to about 50ms.
Benchmark results on Git repo:
```
::(v1.0.0..v2.40.0) 55.5% faster
author(peff).. 96.8% faster
```
ReadonlyIndexLoadError could be a wrapper of PathError instead, but that isn't
compatible with the abstraction of the current load functions. The inner load
functions may be called with an in-memory buffer.
I'll add another directory for changed-path index. We could put them into the
same (content-addressed) directory, but that would just make debugging harder.
This will help extend stats() for changed-paths index. Since there are no
non-test callers who need to calculate stats of mutable index, it should be good
to move the implementation to DefaultReadonlyIndex.
DefaultMutableIndex will calculate diffs to index changed files if enabled. The
body of DefaultMutableIndex::add_commit() is moved to non-trait method. It will
become an async function.
With the current test samples, it seems we need to run ~1000 cases to catch
`heads(first_ancestors())` bug that existed in the original PR. This parameter
can be set by default, but I don't have a good idea about reasonable "cases"
value, prop_recursive() parameters, etc.
Negated `first_ancestors()` can't be converted to a range currently, so
it doesn't make sense to move it to the start when optimizing the order
of an intersection.
This optimization is already done for `heads(ancestors(x) & ...)`, so I
think it makes sense to extend it to `first_ancestors()` as well. This
is especially important if the expression involves a filter that
requires reading commits. For instance, in the nixpkgs repo, evaluating
`heads(first_ancestors(@) & description(jujutsu))` takes 2.6 seconds on
my machine before this optimization, and only 0.2 seconds after.
Using a range for this is consistent with the filter for parent count,
which also uses a `Range<u32>`. This could also be useful to implement
an `nth_parent()` revset in the future.
The filtering is only used for the wanted queue but not the unwanted
queue, which means this can't be implemented using a custom index
implementation either. I think it only makes sense to filter the wanted
queue, because it's unlikely a user would want to exclude only the first
parents of a root commit for a range.
`set_env` is, for various reasons, fundamentally unsafe on approximately
~all modern unicies, and seems like it will never ever be fixed. The
long and short of this is that it will result in segfaults or UB. Rust
2024 therefore marks this function (correctly) as `unsafe`.
The correct solution for 98% of use cases is just to use `envp` during
calls to `execv`, but for our simple cases here of making Git hermetic
there shouldn't be an issue, and a larger refactoring would be needed
for an alternative anyway.
Signed-off-by: Austin Seipp <aseipp@pobox.com>
`gen` is now a reserved keyword; this also matches the surrounding use
of the word `generation` anyway, so IMO it's clearer.
Signed-off-by: Austin Seipp <aseipp@pobox.com>
