Alternative formatting processing

[cdesaintguilhem]

Hi there, as I was tinkering to figure out how to wrap closer to the given wrapping parameter one thing led to another and I arrived at an alternative logic for some of the steps that the formatting loop makes. I thought I'd share the code here in case you would be interested; if there are particular changes you like, I would be happy to cherry-pick them into their own PR and close this one up for the archives.

All the changes in this PR are to the logic only and not to the formatting style, all tests still pass unchanged (save for the addition of a \begin{env1} inside a comment to check that it is properly ignored). However, with these changes, I think that wrapping tightly to the wrapping parameter can be achieved by changing the wrapping_boundary to use args.wrap instead of args.wrap_min. I'm still testing this on another branch, but this requires changing all the test file to account for the tighter expected wrap.

Summary of changes

1. The main formatting loop processes (splits and wraps) lines only by reference, allocating new memory only when applying the indentation, or when returning lines to the queue after wrapping them.
2. To avoid allocating when indenting only to return the line to the queue without adding it to the text, calculating the indent is separated from applying the indent. This allows moving the indent application until after the wrapping computation.
3. Since the indent is no longer applied to the line before wrapping it, the wrapping logic is expanded to instead take into account the calculated indent when deciding whether a line needs to be wrapped and, if so, where it should be wrapped.
4. Since some languages have 0-width chars (such as the French accents `,´,ˆ, etc.), I suspect that going with .chars() could cause inconsistent wrapping in non-English languages. This code introduces the unicode-width dependency to obtain the on-screen width of each character in the string and calculate a wrapping point that visually matches the wrapping parameter.
5. The splitting of lines is done with a new capturing regex from which the index of the matches can be extracted. This then allows returning two string slices: one containing everything before the split point, and the other containing the rest. This Regex could be easily expanded to capture other commands at which the line should be split (such as \section{...} or \[ for example).
6. The wrapping of lines is also done by reference by returning a string slice for the line before the wrap point, a slice containing the start of the next line (to add "% " in front of comments), and a slice containing the rest of the line.

Misc changes

1. Cleaning the text (removing tabs and trailing whitespace) is extracted into a function.
2. Deciding whether to ignore a line (based on verbatim environments or tex-fmt ignore commands) is extracted into a function, and checked before proceeding to the formatting logic.
3. Checking whether formatting returns to zero is done by checking the value of indent.actual after exiting the loop instead of accessing the lines of the text.
4. The test_source() function is modified slightly to panic on the first file that formats incorrectly and to print the file name. The computation of the diff was taking a very long time when many files were failing because of many diffs.

Performance Report

As show from the output below, these changes bring a (63.0 -> 57.1 = ) 9.3% improvement in performance on my machine.

Switched to branch 'main'
Your branch is up to date with 'origin/main'.
Building release binary
   Compiling tex-fmt v0.4.5 (/Users/cyprien/Code/tex-fmt)
    Finished `release` profile [optimized] target(s) in 11.74s
Running benchmark
Benchmark 1: tex-fmt-main
  Time (mean ± σ):      63.0 ms ±   0.7 ms    [User: 58.7 ms, System: 3.8 ms]
  Range (min … max):    62.4 ms …  72.6 ms    200 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs.
 
branch 'pr-refactor-logic' set up to track 'origin/pr-refactor-logic'.
Switched to a new branch 'pr-refactor-logic'
Building release binary
  Downloaded unicode-width v0.2.0
  Downloaded 1 crate (271.5 KB) in 0.21s
   Compiling unicode-width v0.2.0
   Compiling tex-fmt v0.4.5 (/Users/cyprien/Code/tex-fmt)
    Finished `release` profile [optimized] target(s) in 11.78s
Running benchmark
Benchmark 1: tex-fmt-branch
  Time (mean ± σ):      57.1 ms ±   0.5 ms    [User: 53.0 ms, System: 3.8 ms]
  Range (min … max):    56.5 ms …  61.6 ms    200 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs.
 
tex-fmt-main: 0.062972017995s
tex-fmt-branch: 0.05712572198499997s

[WGUNDERWOOD]

Thanks, this is very exciting! I will look through this carefully soon; your contributions are greatly appreciated.

[WGUNDERWOOD]

Other than the comment above, this all looks good to me. Perhaps you can rebase it onto main, noting that I have split parse.rs into cli.rs and read.rs to separate the CLI logic from the file reading logic.

[cdesaintguilhem]

I've now merged your latest changes to main into this branch and addressed the typo.

I'm happy to squash all of these commits into one on my end, but I'm wondering whether this is something that you're also able to do on your side. Up to you if you want to decide what the merge commit should say or if you're happy for me come up with something.

[WGUNDERWOOD]

Yes I think it's best if you squash on your side before merging. Also if you ever want to dig deeper into performance issues, you can run the extra/prof.sh script to produce a flamegraph. Right now it seems that the .count() method in the get_diff function in indent.rs is the main candidate for improvement.

[cdesaintguilhem]

Squash completed.

I've kept the messages in the squash commit's message; and I've kept a branch on my fork with the individual commits.

[WGUNDERWOOD]

Could you add an example test of where the old code would fail on zero-width chars? I tried an example with a French accent in tests/source/unicode.tex, but using .width() doesn't seem necessary for this one to pass.

[cdesaintguilhem]

As a first attempt: the following line (with two spaces before "Aujourd'hui" and where the 70th visual column is between "vérifier" and "is")

y̆es  Aujourd'hui je me suis baladé déhors. Hier je suis allé vérifier si l'été était arrivé.

is wrapped by tex-fmt 0.4.5 to

y̆es  Aujourd'hui je me suis baladé déhors. Hier je suis allé
vérifier si l'été était arrivé.

So it's wrapped at the 61-st visual place, instead of the expected 70th (because of wrap_min)

However, running the current main with cargo run wraps it to

y̆es  Aujourd'hui je me suis baladé déhors. Hier je suis allé vérifier
si l'été était arrivé.

The issue with what I said in the PR is that French accents are not 0-width characters; indeed letters like é are single characters in their own right, rather than being e + ´. However, introducing the (Hungarian?) letter y̆ causes the difference in behaviour since this is represented with two separate characters.

If there is a performance loss in using the function from unicode-width it might be worth removing it given that it might not affect as many languages as I thought.

[WGUNDERWOOD]

I do see a slight performance degradation using unicode-width. I'll look into it a little more to see how likely we are to encounter problems if we remove it, but my guess is that we don't need to worry about this too much, as LaTeX is designed so that most accents can be expressed using standard characters, such as {\'e} for example.

[cdesaintguilhem]

That's a good point about how LaTeX is meant to offer other ways for the user to express accents, without assuming that the source is UTF-8 encoded. Some of the scripts for which I suspect there could be more 0-width characters would include Mandarin, Hindi, Arabic or maybe Korean or Japanese but I don't know enough about these to check. (I also don't know how much LaTeX is used with these languages.)

One thought I had would be to extract the code that calculates a line's length into a function, and have a feature (something like true-unicode) which controls how that function is implemented (using unicode-width::width() when the feature is enabled, or just .char_indices() when not).

[WGUNDERWOOD]

A list of characters for which the width needs modifying is given in the unicode_width crate docs. My impression is that these will be rarely encountered, so I'll remove this dependency for now.