Discover spec directories without globbing every spec file - #2914
connorshea wants to merge 2 commits into
Conversation
|
I don't have access to any large codebase now, @bquorning do you think you can test such a patch for performance improvements? |
|
On a repo with 28 spec folders and ~1700 spec files, I get these numbers: |
|
I've also tested it on my rails app at work to confirm: Best as I can tell, the spec failures in this PR are unrelated to this change and are present on main as well. |
|
Thanks for the effort. |
This is a valid concern, but I've tested it on a few real codebases and a bunch of synthetic codebases with whatever edge cases I could think of, and it has output the same exact data for every one of them before and after. Even if it's only saving 40ms or 100ms on a large app, that's still significant when you're running 50 parallel CI rspec jobs on every PR, hundreds of times every day. The worst downside if this has an edge case I haven't handled would be that it miscounts things in the rails stats, but I'm fairly confident that shouldn't happen. And I'm confident that there's not any case where this would be capable of causing a crash or anything like that. I'm happy to pull a few more major open source rails apps to confirm the data output is identical if you want me to, though. |
|
In my experience of improving CI wall clock time on large Rails projects, up to hundreds of hours if run sequentially, the effort never gets to improve things that are measured in microseconds. 50% of time is usually spent on factories, a significant lag before RSpec can even run anything - on preparing dependencies, starting up docker containers, running migrations. For browser tests major contributors are db truncation (with a solution that tends to break now and then) and long manual timeouts to wait for JS. Every single thing on this list is measured in seconds, not fractions of. I'm proud and jelous if your project is at a point where you've squeezed all that to the point where there's nothing else left than to optimize RSpec itself, which, with a few exceptions, is the last one to blame for being a performance sink. |
I certainly don't disagree with any of this. This change isn't going to revolutionize anything, but I do think boot time especially is important to get down as low as possible, because it's a cost incurred on every boot, no matter how much you try to parallelize a massive test suite. And it continues to scale with the number of files you have in your suite. I've updated the PR to be as minimal as possible and updated the benchmark timings in the PR description accordingly. This is now a much simpler change, at the cost of a bit of efficiency, but that seems fine to me. |
JonRowe
left a comment
There was a problem hiding this comment.
👋 I'm away at the moment but have had a look at this, I'm inclined to merge this once I've had time to sanity check it has the same behaviour, however it will need to be squashed / rewritten to remove Claude from the commits. Its fine to use LLMs to assist you given you've declared it, however commits should be written by a human author who is responsible for the content.
4a47c0d to
b740629
Compare
|
I've squashed this PR into one commit and removed the co-author attributions accordingly. |
b740629 to
4cde62a
Compare
|
Made one more tweak to make it a bit faster on various real codebases (going back to using globbing instead of Find, but still bailing early in most cases. This is better than the previous iteration because it doesn't cost as much when a Rails app has a large amount of files in support, fixtures, or factories that aren't spec files). I've also updated the PR description with benchmarks from various open source Rails apps. |
Previously, the rspec_rails.code_statistics initializer was enumerating every file under spec with a *_spec.rb name just to find the spec directories that exist. This ran on every boot of a Rails app, so it was done on all Rails 8+ apps whenever tests were run. It also meant that larger repositories would spend more time on this as they grew. It only ends up resulting in maybe ~40ms on my Mac when tested with a decently large production app, but that's a small boot penalty on every test run and CI runners are generally less powerful than my dev machine, so I think it's worth fixing. Closes rspec#2912.
4cde62a to
d3cfb82
Compare
|
The one thing that still stands out here is the change from relative paths to absolute, whilst I'm not sure if this affects anything it is a change in behaviour and not how the Rails internals work which makes me want to align with them |
I've updated it to return the relative path now 👍 |
AI Disclosure: This was generated with help from Claude Code, Opus 5/Fable 5. I have reviewed and tested this change manually, and written this PR description myself.
Fixes #2912.
Previously, the
rspec_rails.code_statisticsinitializer was enumerating every file under every directory inspecwith a*_spec.rbname just to find the spec directories that exist. This ran on every boot of the app, so it was done on all Rails 8+ apps whenever tests were run. It also meant that larger repositories would spend more time on this as they grew. It only ends up resulting in maybe 40ms of difference on my Mac when tested with our large production app at work, but that's a small boot penalty on every test run and CI runners are generally less powerful than my dev machine, so I think it's worth fixing.This change gets the exact same results as the previous implementation, but it stops after the first
_spec.rbfile is found in each directory, and so we can avoid extra work in most cases. I have tested therails statscommand against mastodon, discourse, gitlab, canvas, my own small personal rails app, and our app at work, and gotten the exact same results before and after this PR on all of them, as well as some generated test apps to look for any risk of breakage.Benchmarking, courtesy of Claude, 30 iterations each:
rspec-rails, 2 spec dirs)spec/factories(7,101 dirs, 10,000 spec files)spec/supportwith no specs; one spec buried 5 deep)Benchmark script
The main reason the synthetics are significantly faster is because real apps have directories like
fixturesandfactoriesthat never have spec files in them (so we have to read every file in them, we can't bail early), and every file in those directories has to be checked. We could potentially cut further time off by just excluding those two directories, but it technically comes with the risk of someone having actual tests in one of those two directories. I'm unsure whether Rails/rspec would actually work if you did that, but I don't know for sure and I'd rather not complicate this change.We can also bench the memory allocations and see that a lot less memory allocation churn is needed now that we aren't using a regex or loading nearly as many individual file paths. It's only KB or MB at most and is transient, but still nice for reducing GC pressure a bit: