Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize tests #27

Closed
laniakea64 opened this issue Mar 21, 2023 · 8 comments
Closed

Optimize tests #27

laniakea64 opened this issue Mar 21, 2023 · 8 comments
Labels

Comments

@laniakea64
Copy link
Collaborator

Currently it is rather slow to run the test suite. And it will get even slower as we add more test cases.

Two possible ideas for how we could speed it up:

  1. Since each test file seems to take about the same amount of time, consider consolidating some of the existing test cases into fewer files.

  2. Change the test runner to perform the convert-to-htmls in parallel (with # of parallel jobs = # of physical CPU cores?), and only after all outputs are generated, only then go through and colordiff each output compared to its expected. The colordiff runs should stay sequential, to avoid any potential for confusion due to out-of-order output.

Maybe worth doing both?

@NoahTheDuke
Copy link
Owner

Give either of these a try in a PR and we can discuss improvements.

I suspect consolidation of files will be a best approach. I split them up initially because I don't want earlier syntax to influence later syntax. It's hard to test individual pieces if we have to worry about muddying across sections. I suspect your recent changes have helped with that.

Maybe the kitchen sink file could be a new test file?

@laniakea64
Copy link
Collaborator Author

(1) I have some ideas for and could make as a PR. (2) would be a bit steep for me as I have almost zero Rust knowledge atm - been wanting to learn Rust for years, but haven't had enough of a window to do it (& still don't yet, unfortunately ☹️ )

Maybe the kitchen sink file could be a new test file?

Already done 😉 in fact, regarding consolidating test files, first was going to check whether the addition of kitchen-sink.just has made any other test cases redundant.

@NoahTheDuke
Copy link
Owner

Oh, wonderful. I missed you added it to the test folder. Hell yeah, if you feel like there aren't any overlaps of syntax (places where earlier forms affects the syntax highlighting of the current form), cut out any test files that you think overlap.

@nk9
Copy link

nk9 commented Mar 23, 2023

On 2, do we know how much of the build time is Vim starting and how much is the actual conversion? Probably worth trying to use :argdo with all files to see if that on its own can speed things up substantially.

If we DO end up having to parallelize it, one option is GNU parallel, though that would require installing another dependency. Hopefully not a dealbreaker: parallel is pretty sweet. 😄

@laniakea64
Copy link
Collaborator Author

On 2, do we know how much of the build time is Vim starting and how much is the actual conversion? Probably worth trying to use :argdo with all files to see if that on its own can speed things up substantially.

Good point. I'll look into approximate benchmarking of both ways.

If we DO end up having to parallelize it,

... correct me if I'm wrong, but IIRC from one of the times I started trying to learn Rust, isn't parallelism one of the things Rust is particularly good at? So the test runner could itself perform the parallelism without any additional prerequisites?

@laniakea64
Copy link
Collaborator Author

And in trying to get a baseline benchmark... Are we sure ANY of the slowness is due to "the use of vim's built-in TOhtml", as the README claims? Running the following Python code...

#!/usr/bin/env python3

import os, subprocess, sys, tempfile, time

tmpdir = tempfile.mkdtemp()
print(tmpdir)

ts=time.time()

for fn in sys.argv[1:]:
  env={
    'CASE': fn,
    'OUTPUT': os.path.join(tmpdir, os.path.basename(fn[:-5])+'.output.html'),
  }
  subprocess.run(['vim', '-S', 'convert-to-html.vim'], env=os.environ.copy() | env, check=True)

te=time.time()

print(f'Total time: {te - ts} seconds.')

... like this...

cd vim-just/tests
~/benchmark-tohtml.py cases/*.just

... is consistently reporting times around 1-2 seconds - for the entire test suite, converted sequentially. 😮 And inspecting the temporary directory, it did in fact convert all to html as expected.

Sticking a bunch of extra eprintln! lines in the Rust test runner does indeed point to the Vim command as the culprit of the slowness.

... And this fixes it? -

diff --git a/tests/src/main.rs b/tests/src/main.rs
index 916f22a..480cb4c 100644
--- a/tests/src/main.rs
+++ b/tests/src/main.rs
@@ -59,9 +59,8 @@ fn main() -> io::Result<()> {
       .env("CASE", case)
       .env("OUTPUT", &output)
       .env("HOME", env::current_dir().unwrap())
-      .output()
-      .unwrap()
-      .status;
+      .status()
+      .unwrap();
 
     if !status.success() {
       panic!("Vim failed with status: {}", status);

It improves performance so much that it, alone, maybe enough to call this issue fixed IF it's a good answer (I have no actual idea what I'm doing here? 😨 )

@NoahTheDuke
Copy link
Owner

Oooooh interesting! Is .output processing a bunch of text noise from vim? Might be the reason. If that's reducing the time by that much, open a PR with it!

laniakea64 added a commit that referenced this issue Mar 25, 2023
This was referenced Mar 25, 2023
@laniakea64
Copy link
Collaborator Author

laniakea64 commented Mar 25, 2023

Improving performance of the test suite has been completed in #29 .

Hell yeah, if you feel like there aren't any overlaps of syntax (places where earlier forms affects the syntax highlighting of the current form), cut out any test files that you think overlap.

This, and consolidating some existing test cases, are likely still worth doing. It's no longer for performance reasons though, so best address it in another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants