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

Parallelize syntax tests #61

Merged
merged 3 commits into from
Dec 1, 2023
Merged

Parallelize syntax tests #61

merged 3 commits into from
Dec 1, 2023

Conversation

laniakea64
Copy link
Collaborator

From #53 (comment) -

I dug deeper into

Cargo deps were not updated again ... because doing so this time actually reduced performance. There is a cargo-audit warning that we are using a yanked crate (ahash version 0.8.3, reasoning for the yank explained on their wiki), so the question of updating Cargo deps again should be investigated, but let's address that separately.

... and concluded that performance impact has known reason (at least in part) and we're going to be stuck to eat it at some point. I have a WIP patch attempting to mitigate it,

This pull request is that patch. It's a rayon-based implementation of the test runner change suggested in #27 -

  1. 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.

This makes it harder to use time on the syntax runner as an approximate benchmark of the syntax highlighting itself. However, this ability was useful for development. To accommodate this usage, the test runner now handles this itself: it takes approximate execution time of each Vim process, and prints the grand total sum of all execution times. (The numbers this is printing are consistent with the wall clock times reported by time before these patches.)

With these changes, updating Cargo deps no longer has any measurable performance impact. So that's done here too, to fix the cargo-audit warning.

@laniakea64
Copy link
Collaborator Author

Hi @NoahTheDuke would appreciate your review before merging if possible, will you be available to review this? Thanks

@NoahTheDuke
Copy link
Owner

Go for it!

@laniakea64 laniakea64 merged commit ad6eac0 into main Dec 1, 2023
1 check passed
@laniakea64 laniakea64 deleted the tests-performance branch December 1, 2023 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants