perf(test): improve test performance #2473
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Avoid running expensive metadata generation when generating `Document` in `FixedPhrase::from_phrase()`.
Use generate `Document` from simple tokenization in `ProperNounCapitalizationLinter`.
Try to detect memory leak via monitoring memory usage rather than trying to force an OOM in `does_not_oom_with_repeated_lints()`. This significantly reduces the time it takes to run `cargo test`.
elijah-potter
requested changes
Jan 15, 2026
Collaborator
elijah-potter
left a comment
elijah-potter
left a comment
There was a problem hiding this comment.
Overall, this is great work. There are a few questions I need answered before I can merge this. Thanks!
harper-wasm/src/lib.rs
Outdated
| let pid = Pid::from_u32(std::process::id()); | ||
| let mut sys = System::new(); | ||
|
|
||
| macro_rules! refresh_sys { |
Collaborator
There was a problem hiding this comment.
Is there a particular reason this is a macro instead of a function?
Contributor
Author
There was a problem hiding this comment.
I refactored it into a function inside that test module. If there's a better place for it, please let me know.
(iirc I initially tried making it a closure that would capture pid and sys to avoid having to pass them as arguments. Capturing &mut sys made the borrow checker unhappy though, and the closest alternative that came to mind at the time was a macro.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Reduces the amount of time it takes to run
cargo test.Before rebasing on master and with only the first two commits, this reduced the amount of time it takes to run
cargo testfrom ~78s to ~57s on my system. This is primarily achieved by not running the chunker/tagger when creating theDocumentinFixedPhrase::from_phrase().After rebasing, the
does_not_oom_with_repeated_lints()test seems to have regressed significantly in performance, and runningcargo testtook about ~180s. I took a quick crack at trying to resolve this in 0659fab. This reduced thecargo testtime to ~53s, albeit I'm not 100% certain that the test will still work as intended (I'm not super familiar with Wasm).(The regressions in
does_not_oom_with_repeated_lints()seemed to have been caused bylex_email_address()andlex_url()and the way they're used in some manner related to Weir. It seemed to be caused by the iterators at the beginning of those functions trying to find the ':' for the URL, or the '@' for the email. Replacing those iterators with a loop and adding early exits seemed to significantly reduce the impact of this. Without any changes that test took >100s, with those early exits it took ~37s, and with 0659fab it takes about ~1.5s. I didn't spend too much time investigating why this became so much of an issue after Weir was added, but I think it could be an interesting direction for further optimization.)How Has This Been Tested?
cargo testChecklist