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

Process multiple files in parallel #770

Merged
merged 12 commits into from
Feb 11, 2023
Merged

Process multiple files in parallel #770

merged 12 commits into from
Feb 11, 2023

Conversation

lydell
Copy link
Contributor

@lydell lydell commented Aug 28, 2022

Fixes #755. Related to #183.

On my computer, it’s roughly 2.5 times faster! 🎉

command main this PR
elm-format --validate . 7.7 s 2.9 s
elm-format --yes . >/dev/null 7.7 s 2.9 s
elm-format --yes . 7.7 s 3.3 s

main refers to commit 1557892.

Tested on a code base of this size:

❯ tokei --type Elm
===============================================================================
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Elm                   745       221612       178984         5131        37497

Computer specs:

  • macOS Monterey
  • CPU: 2,6 GHz 6-Core Intel Core i7
  • RAM: 16 GB 2400 MHz DDR4

Terminal: elm-format --yes . (without >/dev/null) depends on your Terminal. With the default Apple Terminal, I got 3.3 seconds as shown in the table above. With iTerm2, I got 5.5 seconds (and 9.7 seconds for main).

Notes:

  • This PR seems to work, but I haven’t tested all aspects extensively.
  • I have only tested on macOS, not Linux or Windows.
  • I’m a Haskell beginner, so I’ve probably done some weird things. I even know some of them: I generate JSON for the list of validation objects a bit manually, and I turn errors into JSON earlier than feels logical to avoid a “deriving NFData hell” that I couldn’t figure out. I took all shortcuts I could just to win over the compiler :)
  • It feels like I completely changed how InfoFormatter works. Sorry if that doesn’t fit with your vision for it!
  • I use @supermario’s hack to avoid garbled printouts: https://github.com/supermario/elm-tooling-compiler/blob/0f63c27e0f334ca680a568034953f9b3d16ba3db/ext-common/Ext/Common.hs#L82-L94
  • I didn’t spend much time thinking about code style. It’s probably all over the place and not aligned with the rest of the code base!
  • I use pooled-io which is on version 0.0.2.2 – that’s a very low number! I picked that dependency because it was the only one I understood how to use. It is only used to implement World.mapMConcurrently (a one-liner) though, so it might be easy to replace by someone better at Haskell.

@avh4 avh4 added this to the 0.8.6 milestone Aug 29, 2022
@avh4
Copy link
Owner

avh4 commented Aug 29, 2022

Awesome, thanks!!

  • Linux: I'll be able to test that
  • Windows: I'll make sure to have someone check a release candidate build before the next release (or I guess I need to make github CI save the windows artifacts for PRs)
  • pooled-io 0.0.2.2: low version number doesn't bother me too much, as long as the library is pretty small and self-contained. Also in this case, I did look at pooled-io's source code, and it looks pretty straightforward as a wrapper over more common Haskell concurrency APIs, so this seems totally fine to me
  • code style: I haven't really settled on a consistent one anyway 😅. Everything I saw so far looks good for code style.
  • InfoFormatter: I've been trying to get rid of it for a while, so I'm pretty much fine with anything here

Apologies, I'll probably be a little bit slow on merging this since I want to re-read the InfoFormatter changes a couple times, and make sure I personally understand how Haskell's laziness works w/r to the new code. (I trust that doesn't really break anything, both from a quick skim of the code, and from the testing you've done; but I want to put a bit higher of a bar on myself to understand what the runtime behavior actually is.)

Here are a couple things I'll want to take a look at before I merge. Feel free to take a stab at them if you want, or no worries if you don't, I should have a chance to get to them in the next couple weeks.

  • I'm thinking using the print lock, it should be possible now to have the forked jobs just print their output, and then only have to pass back a Bool to the parent, and that maybe also would restore the streaming output of the json
  • I'd like to get rid of the unsafePerformIO hack, which I think I'd do by making a new monad stack for the "real" World instance, which would be something like type RealWorld = ReaderT (MVar ()) IO, make a MonadIO RealWorld instance (maybe? I think this might make it less verbose to refactor the World instance? -- edit: oh, maybe ReaderT r IO already has a MonadIO instance?), and then refactor instance World IO to be instance World RealWorld (and the top-level entrypoint then will create the printLock and use runReaderT printLock)
  • (probably won't look at now for this PR, but might someday in the future) rewrite TransformFiles to use conduit, and then use conduit-concurrent-map instead of pooled-io.

@avh4 avh4 self-assigned this Jan 26, 2023
@avh4
Copy link
Owner

avh4 commented Feb 10, 2023

Testing version for MacOS arm64 is available here: https://github.com/avh4/elm-format/actions/runs/4141690402

@avh4
Copy link
Owner

avh4 commented Feb 10, 2023

Just an additional benchmark on a Mac M1 using --validate on ~150k LOC:

(using https://github.com/sharkdp/hyperfine hyperfine --warmup 3 --shell=none)

Version Mean [s] Min [s] Max [s] Relative
darwin-arm64
this PR 0.824 ± 0.006 0.815 0.832 1.00
main 2.776 ± 0.015 2.758 2.805 3.37 ± 0.03
darwin-x86 via Rosetta
this PR 1.058 ± 0.012 1.042 1.085 1.28 ± 0.02
main 9.603 ± 0.096 9.475 9.770 11.65 ± 0.14

@avh4
Copy link
Owner

avh4 commented Feb 11, 2023

I'm thinking using the print lock, it should be possible now to have the forked jobs just print their output, and then only have to pass back a Bool to the parent, and that maybe also would restore the streaming output of the json

I took a brief look at doing this now, but it doesn't seem worth the time atm. Doing it in a clean way seems like it would require a way of coordinating an additional thread, which would fit better with the possible future work of using conduit.

This was referenced Feb 11, 2023
avh4 added a commit that referenced this pull request Feb 11, 2023
@avh4 avh4 merged commit 868eb08 into avh4:main Feb 11, 2023
@avh4
Copy link
Owner

avh4 commented Feb 11, 2023

Split off #788 for possible future refactoring.

@avh4
Copy link
Owner

avh4 commented Feb 11, 2023

Merged via #789

Thank you!!

@lydell lydell deleted the concurrent branch February 11, 2023 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

process multiple files in parallel
2 participants