Format files in parallel

[michaelpj]

Closes #1128

There is no explicit configuration for this, but it is possible for users to override by changing the RTS options to the ormolu executable.

[mrkkrp]

Thanks! I imagine you have verified that this indeed works and results in performance benefits? We have some checks on the format of the .cabal file, could you fix the lint issue by running cabal format?

[amesgen]

We have some checks on the format of the .cabal file, could you fix the lint issue by running cabal format?

Not directly related to this PR, but I just got reminded that we use cabal format which is removing the comments; we could switch to eg cabal-gild which preserves this (and is in general much better than cabal format). Any objections/shall I create a PR switching to that?

[amesgen]

Some comments (mostly for me to look at later):

• Support multithreading #896 (comment) talks about garbled output. We should check that, and either add some synchronization or check that -N1 produces ungarbled output and tell users to use that if they want ungarbled output.

• There could be races here:

  ormolu/src/Ormolu/Utils/IO.hs

  Lines 53 to 61 in 7a334ac

  	withIORefCache :: (Ord k) => IORef (Map k v) -> k -> IO v -> IO v
  	withIORefCache cacheRef k action = do
  	cache <- readIORef cacheRef
  	case M.lookup k cache of
  	Just v -> pure v
  	Nothing -> do
  	v <- action
  	modifyIORef' cacheRef (M.insert k v)
  	pure v

  
  I think we don't need to change sth here for correctness, but there actually might be some redundant work when multiple threads try to compute the value for the same cache key?

  We could also move away from shared NOINLINE ad-hoc caches, and instead compute such data upfront (see also the last paragraph in Cache .cabal file parsing and processing #915).

[michaelpj]

I imagine you have verified that this indeed works and results in performance benefits?

Yes, as I mentioned here I got a pretty good speedup (3x) on my usecase (formatting about 2.5k files).

We should check that, and either add some synchronization or check that -N1 produces ungarbled output and tell users to use that if they want ungarbled output.

Ah right, I have only been checking with --mode inplace. This is a bit awkward, since in principle we need to lock around all terminal writes. I think all the writes happen in app/Main.hs, so perhaps we can still do this reasonably cheaply.

I think we don't need to change sth here for correctness, but there actually might be some redundant work when multiple threads try to compute the value for the same cache key?

We could also just make this thread-safe by using an MVar instead of an IORef, right?

[amesgen]

We should check that, and either add some synchronization or check that -N1 produces ungarbled output and tell users to use that if they want ungarbled output.

Ah right, I have only been checking with --mode inplace.

Maybe we could only do parallel processing for --mode inplace? 🤔

This is a bit awkward, since in principle we need to lock around all terminal writes. I think all the writes happen in app/Main.hs, so perhaps we can still do this reasonably cheaply.

We have this:

ormolu/src/Ormolu/Fixity/Internal.hs

Lines 275 to 276 in 7a334ac

	trace
	(renderFixityJustification opName moduleName m result)

But it is only emitted when --debug is set.

I think we don't need to change sth here for correctness, but there actually might be some redundant work when multiple threads try to compute the value for the same cache key?

We could also just make this thread-safe by using an MVar instead of an IORef, right?

Yes 👍

[michaelpj]

Maybe we could only do parallel processing for --mode inplace? 🤔

I think it would be nice if it worked for --mode check too. TBH, I think --mode stdin is a bit questionable with multiple files even without parallelism... do people use that?

[amesgen]

Maybe we could only do parallel processing for --mode inplace? 🤔

I think it would be nice if it worked for --mode check too.

Oh yeah, --mode check should also work with parallelism, definitely 👍

TBH, I think --mode stdin is a bit questionable with multiple files even without parallelism... do people use that?

Yeah, agreed, I can't imagine a use case.

[mrkkrp]

Any objections/shall I create a PR switching to that?

Indeed, it would be nice to have a formatter for .cabal files that preserves comments.

[michaelpj]

• I added locking around writing to stdout and I don't get garbled output
• I made the cache thread-safe. This required changing some types which is a breaking change to the Utils module, but I can't see a way around it since the IORef is exposed in the type. My suspicion is that nobody is using Utils even though it's exposed on Hackage, so I guess it's a judgment call whether you're okay with this.

I've continued to allow formatting multiple files with mode stdin, it's just going to be undefined what order you get the output in. I guess in principle if people were using this they might care about the order, but perhaps we can just wait and see if anyone complains? Fixing it would be more work for probably no users.

[michaelpj]

Also this should probably be documented, not sure where.

[mrkkrp]

• which is a breaking change to the Utils module

I think we only consider the Ormolu module as stable, so changes in Ormolu.Utils.IO should be fine.

I've continued to allow formatting multiple files with mode stdin, it's just going to be undefined what order you get the output in. I guess in principle if people were using this they might care about the order, but perhaps we can just wait and see if anyone complains? Fixing it would be more work for probably no users.

I think it is totally fine. If anyone uses formatting through stdin on multiple files they can complain in the issue tracker. Not sure we even need to document that.

[mrkkrp]

@amesgen I think this is ready to be merged. What do you think?

[amesgen]

Looks good, two minor comments

I tested this on Agda (ie running ormolu -m check $(fd -e hs . $(nix build .#hackage.Agda --print-out-paths))), and execution time drops from ~20s to ~7s on my machine with 8 cores 🎉

[mrkkrp]

Thanks!