Change HscEnv.hsc_HPT from HomePackageTable to IORef HomePackageTable
AbandonedPublic

Authored by duog on Aug 7 2017, 5:52 PM.

Details

Reviewers
hvr
bgamari
ezyang
austin
Trac Issues
#14095
Summary

This Revision is a work in progress, and I am mainly submitting it at
this point for validation. There will be several more Diffs building
on this one to implement the design described in the ticket.

This should result in no change in behaviour.

This change is to support Trac #14095: Improve parallelism in --make mode

duog created this revision.Aug 7 2017, 5:52 PM
duog updated this revision to Diff 13460.Aug 7 2017, 6:50 PM

ensure linkables get added to the hpt

duog updated this revision to Diff 13461.Aug 7 2017, 9:06 PM

Ensure linkables are added to the hpt, even for ghci!

duog updated this revision to Diff 13465.Aug 9 2017, 5:08 PM

Rebase onto D3815

duog updated this revision to Diff 13466.Aug 9 2017, 5:21 PM

Oops, getting used to stacked diffs

bgamari requested changes to this revision.Aug 18 2017, 9:33 AM

Bumping out of the review queue. Let me know when you want a review, @duog.

This revision now requires changes to proceed.Aug 18 2017, 9:33 AM
duog added a comment.Aug 21 2017, 3:42 PM

Hi @bgamari , I would like some feedback on the following:

  • Are the stacked Diffs I have used helpful, or would it be better to put all three commits in a single Diff?
  • Is the idea of putting the home package table into an IORef and adding yielding callbacks to HscEnv reasonable?
bgamari added a subscriber: ezyang.Aug 22 2017, 9:48 AM
In D3832#108878, @duog wrote:

Hi @bgamari , I would like some feedback on the following:

  • Are the stacked Diffs I have used helpful, or would it be better to put all three commits in a single Diff?

They are very useful, thank you! I find that the difficult of reviewing a patch scales superlinearly with its size.

  • Is the idea of putting the home package table into an IORef and adding yielding callbacks to HscEnv reasonable?

Well, there is certainly precendent. I think it makes sense to treat the EPS and HPT symmetrically. While the functional programmer in me isn't entirely happy with this situation, the pragmatist in me believes this is probably the best solution given where we are today. However, I would also like to hear @ezyang's opinion on this.

compiler/main/HscTypes.hs
407

We should make sure this gets fixed.

676

Whitespace

duog added a comment.Aug 22 2017, 4:47 PM

Thanks!

compiler/main/HscTypes.hs
407

Absolutely, I mark things with DOUG so I can grep for them and make sure they get fixed.

It would be easier to give a judgment on HPT in an IORef if I knew what design forces conspired to put you into this regime. This particular change comes with the risk that some callers are hanging on to HPT because they believe it won't update, when it actually does.

duog added a comment.Aug 27 2017, 6:18 PM

It would be easier to give a judgment on HPT in an IORef if I knew what design forces conspired to put you into this regime. This particular change comes with the risk that some callers are hanging on to HPT because they believe it won't update, when it actually does.

In current master it is a precondition of DriverPipeline.CompileOne' that any imported modules are in the home package table, and this is the impediment to parallelization that I am trying to improve. Then there are two obvious approaches:

  • Split CompileOne' into several functions, one each for parse, typecheck, simplify and codegen. Each of these functions would take an immutable home package table, and have a precondition that any imported module would have "enough" information in that home package table. Then the "MakePipeline" I have defined could be controlled in DriverPipeline. I think if we were starting from a blank slate this might be the best option, however I didn't go this way (initially anyway) because the required changes were much greater.
  • Loosen the precondition that the home package table be complete, either by using a mutable variable, or by threading a home package table through all the necessary functions. I think the latter option could be implemented relatively cleanly by redefining
newtype Hsc a = Hsc (HscEnv -> WarningMessages -> IO (a, HomePackageTable, WarningMessages))

and updating hsc_HPT in runHsc. I went with the mutable variable option because it seemed the simplest way to get a working prototype that I could benchmark (still working on that!), there are a lot of call-sites for runHsc, it's not obvious how to thread through the updated home package table in all of them. I don't think there is a difference between the mutable variable and state monad approaches in the risk of someone holding onto an home package table.

ezyang requested changes to this revision.Sep 30 2017, 4:49 PM

duog and I chatted about this offline, and we came up with a scenario where this change is unsound.

The basic question relates to the thread-safety of our atomicModifyIORefs and readIORefs throughout GHC's codebase. During the compilation of a module, GHC may query for the HPT by readIORef multiple times, and we would expect all the HomeModInfos that it actually reads stay the same. In the absence of loop retypechecking, the HPT grows monotonically and so even if we modify the HPT, things are only added, not modified.

However, a loop retypecheck means that GHC may update a module that already lives in the HPT. And now the hazard is that, *within the compilation of a single module*, the HomeModInfos of the modules that the module depends may change without warning, and we don't want to be in the business of ensuring GHC works even if such a thing happens.

One extra complication is that, we thought perhaps the fact that parallel recompilation waits for module loops to finish before continuing typechecking/compiling saves one from this situation. But this doesn't block in all situations (e.g., if A and B mutually depend on each other, they are both loop breakers, but you can't BOTH block A on B and B on A: that's a deadlock) and so you can still end up with the intermediate update.

duog has a plan for implementing his changes without needing the IORef, and is going to do it.

austin resigned from this revision.Nov 9 2017, 5:36 PM
duog abandoned this revision.Jan 1 2018, 4:30 PM

Reworking. See D3836