spoom icon indicating copy to clipboard operation
spoom copied to clipboard

Very slow because of file scanning, and does not respect sorbet config

Open bmulholland opened this issue 3 years ago • 2 comments

Describe the bug Running spoom bump takes a very long time for us (>2 mins on a small codebase). Root cause is we use ActiveStorage, which in dev mode creates a lot of directories and sub-directories. Spoom expands those; this alone line takes 62s to run. https://github.com/Shopify/spoom/blob/e1d339fa287ee5796d252cda3e7c0eb7adc6bfa7/lib/spoom/sorbet/sigils.rb#L91

Ideally I'd just ignore the storage directory, as I've done for Sorbet, but Spoom doesn't have a way to ignore those directories, despite what the docs claim.

bundle exec spoom bump --from ignore --to false 3.44s user 34.14s system 26% cpu 2:21.30 total

To Reproduce Spoom version: master

Create lots of dirs and subdirs in tmp and run spoom bump

Expected behavior Does not recursively scan through directories ignored in sorbet config.

bmulholland avatar Apr 11 '22 12:04 bmulholland

👋 Hey @bmulholland,

Thanks for the report, this is indeed unfortunate... I tried a slightly more efficient way of doing so in https://github.com/Shopify/spoom/pull/128. Do you mind testing and reporting any improvement or new problem? 🙏

Morriar avatar Apr 12 '22 22:04 Morriar

Thanks for trying to fix! Unfortunately that doesn't solve the problem because Spoom::Sorbet.srb_files uses essentially the same code under the hood: https://github.com/Shopify/spoom/blob/4f94563d442fb071c6e4221c73efecfae1d2d05b/lib/spoom/sorbet.rb#L58

bmulholland avatar Apr 13 '22 09:04 bmulholland

@bmulholland I gave it another try in https://github.com/Shopify/spoom/pull/322 if you don't mind to test 🙏

Morriar avatar Mar 22 '23 22:03 Morriar

I recently reset my local database and haven't done much development since, so I "only" have 39k files & dirs in storage/ (find storage | wc -l => 39703). spoom runs in ~2.5s with or without the branch. I do get an error when running tapioca gem: "undefined method `srb' for Spoom::Sorbet:Module (NoMethodError)"

bmulholland avatar Mar 24 '23 10:03 bmulholland

Hey @bmulholland,

Regarding the Tapioca error this is expected, I did a few changes in the API of Spoom and will have to reflect them on Tapioca before releasing a new version.

Concerning Spoom, something else must be at play for it to be so slow. Here's the test I did:

  1. Created a few hundreds of files under tmp/ with typed: false
  2. Added tmp/ to my sorbet/ignore config
  3. Ran spoom bump --from false --to true
  4. Checked which paths where scanned by Spoom with a few puts

Here's the Sorbet config I used for the test:

.
--ignore=tmp/

And I can't see any of the ignored paths being discovered.

Do you mind sharing you sorbet/config file with me? Maybe it's about the format used to ignore the paths?

I also created a new branch that prints the paths being discovered: https://github.com/Shopify/spoom/compare/at-context-file-tree-test?expand=1 can you try again with this branch and see if the output gives you an indication of what is being scanned? That may help.

Thanks a lot!

Morriar avatar Mar 27 '23 14:03 Morriar

A bit late, but tried out the branch and confirmed it's not listing out any of the ignored paths listed as ignore in sorbet/config. (It does list the directories, but not anything inside them.) It's still taking about 2.5s, is that slower than expected?

bmulholland avatar Apr 18 '23 14:04 bmulholland

Around 2 seconds seems plausible. To do the bump, Spoom will:

  1. Parse the Sorbet config
  2. List all the files matching the config
  3. Read all the listed files to find the one having the from sigil
  4. Write all the files that need to be bumped to the to sigil
  5. Run Sorbet on the whole codebase (this step may take some time, especially if bumping creates a huge error output)
  6. Parse the output of Sorbet to find files with and without errors
  7. Write back the files to their original sigil if they have errors

To be sure we could add times to each step to see where is the time spent.

Morriar avatar Apr 18 '23 15:04 Morriar

As long as 2s seems in line with expectations, I'd say this issue is fixed now. Thank you!

bmulholland avatar Apr 18 '23 16:04 bmulholland