This is a surprisingly large perf win. On my Thinkpad:
typing_before/after.ml:
before: 3.038B instructions
after: 2.870B instructions
slow_before/after.rs:
before: 2.381B instructions
after: 1.260B instructions (!)
Vertex is allocated on the arena, so it is never dropped;
then it cannot contain a Vec allocated on the regular heap
without leaking memory. Replace the Vec with a slice allocated
on the arena, which seems to fix most of the leaks. (Some may
remain; I haven't checked fully.) It should also be slightly
more memory-efficient.
It's not clear that we actually need the RefCell instead of
just putting Option directly into the structure, but I've
let it stay.
This issue was probably introduced in a71d6118cf.
This isn't strictly necessary since difftastic is a binary-only
crate. However, it improves compiler warnings (see next commit) and
potentially helps future changes to make difftastic available as a
library.
This corresponds to:
$ cargo +nightly fmt -- --config group_imports=StdExternalCrate
Since this option is only available on nightly, I'm not adding a
rustfmt.toml to enforce this, just doing it as a one-off run.
This saves us searching the hash map twice. This is a modest
performance improvement: an instruction count reduction of 4% on
slow_before.rs, and 1% reduction on typing_before.ml.
This was intended to allow usage of .entry_ref(), but it's already a
performance win without using that API! It's around a 9% reduction in
instructions in slow_before.rs, and 2% reduction in typing_before.ml.
If a string is replaced with another, apply subword highlighting
similar to how we handle replaced comments.
Co-authored-by: Wilfred Hughes <me@wilfred.me.uk>
The contiguous penalty was an attempt to fix the slider problem:
// Old
A B
C D
// New
A B
A B
C D
// Unwanted diff
A +B+
+A+ B
C D
However, it doesn't make sense for Dijkstra, which is stateless. The
best route from vertex X is independent of how we got to vertex X.
This worked by dumb luck: in some circumstances we terminate early
rather than fully executing Dijkstra's algorithm. This cost tweak
improved results on a few test files. However, the post-processing
slider logic is a proper, general solution. This was added much later.
There's no reason to keep the contiguous penalty now. It's confusing,
and makes adding new edge costs with consistent 'X costs more than Y'
behaviours more difficult.
Performance is essentially neutral: a small decrease in
typing_before.ml, a small increase in slow_before.rs.
This ensures that choosing a unchanged non-punctuation atom with some
novel atoms is better than choosing punctuation and some changed
comments. This produces better results in general, see
comma_and_comment_after.js for an example.
This will be more noticeable after the next commit, where costs of
novel atoms are in a smaller range of values.
This is harder to reason about, and
2e6666041f did not include a motivating
test case.
Removing contiguous status is a minor perf improvement (2% reduction
in instructions), makes the code simpler, and does not significantly
affect diffing results.
Of the two sample files that have changed, the erlang_before.erl file
has improved and nest_before.rs is neutral.
This is equivalent (increased cost on unchanged nodes vs decreased
cost on changed nodes), but easier to reason about.
Previously we have multiple notions of changed atoms: NovelAtomLHS,
NovelAtomRHS, and ReplacedComment. We want to consider punctuation as
less desirable even when e.g. comments arereplaced.
@QuarticCat observed that popping delimiters is unnecessary, and saw a
speedup in PR #401. This reduces the number of nodes in typical graphs
by ~20%, reducing runtime and memory usage.
This works because there is only one thing we can do at the end of a
list: pop the delimiter. The syntax node on the other side does not
give us more options, we have at most one. Popping all the delimiters
as soon as possible is equivalent, and produces the same graph route.
This change has also slightly changed the output of
samples_files/slow_after.rs, producing a better (more minimal)
diff. This is probably luck, due to the path-dependent nature of the
route solving logic, but it's a positive sign.
A huge thanks to @QuarticCat for their contributions, this is a huge
speedup.
Co-authored-by: QuarticCat <QuarticCat@pm.me>
This produces substantially better diff results, and fixes the 'last
item in the list shown as changed' problem.
This can produce slower diffing. typing_before.ml takes 10% more
instructions and slow_before.rs takes 110% more instructions.
This is a more traditional graph representation. It is slightly easier
to reason about, and it's clearer that graph node creation time
dominates graphs exploration.
This is a slight performance regression, but it enables better
exploration of parethesis nesting (see next commit). typing_before.ml
has regressed from 3.75B instructions to 3.85B instructions and
slow_before.rs has regressed from 1.73B instructions to 2.15B
instructions.
This change has also made the diff output for slow_before.rs slightly
worse (note the `lhs` variable is now claimed as changed in more
cases). It's not clear why, but presumably means that the node visit
order has changed slightly.
Closes#324
Wilfred Hughes
Steinar H. Gunderson
Zhenge Chen
QuarticCat