Move ClosureType from ghc-heap into GHC.ClosureTypes

[mpickering]

GHC MR: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9560

The whereFrom function defined in GHC.InfoProv returns the IPE information for a specific closure. The InfoProv data type used to represent the closure type of said closure using an integer. From a user perspective it's better to decode this and represent the closure type as an ADT.

Fortunately such an ADT already exists but it is defined in ghc-heap, so we have to move the definition of this ADT into base so it can be used by whereFrom and also ghc-heap.

With this change the closure type can be properly represented using ClosureType for anyone using the whereFrom function.

[Bodigrim]

To start with, the addition of a new public module GHC.InfoProv is not reflected in base-4.18 changelog (and, being pedantic, there was no proposal for this). Does it need to be exposed? whereFrom is already available from GHC.Stack.CCS.

[TeofilC]

It looks like whereFrom was moved to GHC.InfoProv as part of: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/8868

[TeofilC]

I've noticed that the commit that removes whereFrom from GHC.Stack.CCS is present in the GHC-9.6 branch, so, it should probably be decided whether that is alright before 9.6.1 goes out.

[Bodigrim]

I've noticed that the commit that removes whereFrom from GHC.Stack.CCS is present in the GHC-9.6 branch

...and missing from base-4.18 changelog. @mpickering could this please be sorted out?

[mpickering]

@Bodigrim Please can you open a GHC ticket if there is a problem with GHC which needs resolution?

@TeofilC As far as I am aware it has only been communicated to GHC developers that CLC policy within the last two weeks that ALL changes to base have to go through this process. That change to `whereFrom lies firmly outside the statute of limitiations.

This ticket is solely about the proposal to add ClosureType into the GHC.ClosureTypes module, so please can we focus on starting that discussion so that we can make progress on this issue on the GHC side?

[TeofilC]

That makes sense. Thanks for that context @mpickering!

[Bodigrim]

As far as I am aware it has only been communicated to GHC developers that CLC policy within the last two weeks that ALL changes to base have to go through this process. That change to whereFrom lies firmly outside the statute of limitiations.

There is certainly a miscommunication.

Back in November 2021 Simon suggested CLC to monitor all, even minute changes to base such as documentation (#20 (comment)). I asked to narrow the scope to only client-visible changes (#20 (comment)). The thread continued in December 2021 by email with Simon and Ben (https://groups.google.com/g/haskell-core-libraries/c/hSl5ZBmSgLs/m/ar2peyZWBwAJ): Simon insisted again that CLC should maintain "all aspects", which we felt too intrusive for the workflow of GHC developers and contributors. The clarification of responsibilies has been proposed in February 2022 (#38), which Simon reviewed in detail. There've been further discussion in November 2022, where I encouraged GHC developers to nominate modules to be excluded from CLC scrutiny (#105 (comment)), but none were nominated.

Bottom line is that CLC oversees less than it was suggested by GHC developers, and the scope has been communicated multiple times. There've been no recent change of policy, certainly not in the last two weeks or so.

This ticket is solely about the proposal to add ClosureType into the GHC.ClosureTypes module, so please can we focus on starting that discussion so that we can make progress on this issue on the GHC side?

Indeed, we are trying to focus on your proposal, which starts with "The whereFrom function defined in GHC.InfoProv returns the IPE information for a specific closure". The problem is that there is no such module in the released version of base-4.17, and whereFrom function is to be found only in GHC.Stack.CCS. I don't know whether this is the same whereFrom you are talking about or no, so I looked into changelog.md to find out, but it is absolutely silent both about whereFrom and GHC.InfoProv. Thus I'm stuck and asking for clarifications. Please bring changelog up-to-date, so that we can continue the discussion of your proposal.

I've raised https://gitlab.haskell.org/ghc/ghc/-/issues/22883 to help tracking this on GHC side.

Matthew, I'm happy to jump on a brief call if it helps.

[Bodigrim]

Back to the proposal, this is a breaking change, but GHC.InfoProv has been just introduced in base-4.18 and is not to be used widely. I assume consumers are limited to semi-internal tooling for heap profiling, which is very much unstable anyways. So I'm +1 on the proposal.

Dear CLC members, any opinions on this? (This is not a vote yet)

@TeofilC could you please update the MR fixing suggestions?

[TeofilC]

I've updated the MR hopefully the new version should resolve all the suggestions

[tomjaguarpaw]

+1 if and only if

1. this is a stable API, or
2. it is not exposed at all (but I assume it must be so it can be re-exported by ghc-heap), or
3. it is changed to be exposed from a module with .Internal in the name.

Additionally, I believe @bgamari is working on a plan for the GHC. hierarchy that takes API stability into account. I wonder if he has any thoughts on this.

[hasufell]

Back to the proposal, this is a breaking change, but GHC.InfoProv has been just introduced in base-4.18 and is not to be used widely.

Depending on how popular 9.6.1 is going to be, it could still happen that large proprietary code bases adopt this type and will break in the next major base release.

Is there a way to manage this with a deprecation phase and a new function?

[TeofilC]

Is there a way to manage this with a deprecation phase and a new function?

I personally don't think this would be worth the effort and added complexity.

Instead I'm thinking about publishing a wherefrom-compat package to Hackage. This would give a consistent interface for 9.4, 9.6, and 9.8 (once it's released). Folks who want a stable interface can depend on that package. This would also help deal with the move from GHC.Stack.CCS to GHC.InfoProv in 9.6.
An entry in the migration guide could point users towards this.

Would that address your concern @hasufell ?

[chshersh]

To summarise the changes w.r.t. to base in this proposal:

1. Addition of the new module GHC.ClosureTypes with the new ClosureType type
   • Potentially breaking change if someone else already defined this module (but I doubt anyone did this)
2. Change of the closureType :: Word field to closureType :: ClosureType in the StackEntry type exported from GHC.Stack.CloneStack module
   • Breaking change
3. Change of the ipDesc :: String field to ipDesc :: ClosureType in the InfoProv type exported from GHC.InfoProv module.
   • Breaking change

So, as we can see, it is a breaking change to the existing public API of base. I don't see an easy backwards-compatible patch here, and I'm okay with just moving forward with it. I don't think this API is popular to justify high backwards-compatibility cost. Besides, I imagine people who use these GHC internals are already used to constant breaking changes so this patch won't be too surprising for them.

So I'm +1 on this.

---

I can empathise with GHC developers seeing the CLC process as unnecessary overhead to their daily work. To sweeten the pill, I can say that a CLC proposal can act as additional documentation to all the changes with the opportunity to receive early feedback from more people.

If GHC developers want to decrease the CLC involvement in the internals of the GHC-specific changes, I can suggest the following paths forward:

• Propose a list of modules to be excluded from CLC supervision (as per @Bodigrim comment)
• Move modules under the GHC.Internal.* namespace
• Move modules to a separate package from base
• Document in the modules that they are unstable, experimental and can change public API without adhering to PVP
• Change fewer things and break less
• Design API initially with forwards-compatibility in mind

As I understand, some of these are already being worked on. But until then, it's mandatory to have a CLC proposal to all changes in the public API of base.

[Bodigrim]

Depending on how popular 9.6.1 is going to be, it could still happen that large proprietary code bases adopt this type and will break in the next major base release.

That's highly unlikely, this stuff is not for business logic, just for diagnostic tools. I strongly suspect that all its users are already present in this thread ;)

If I'm not mistaken, the current version of the API has never been externally reviewed, and IMO it makes little sense to enshrine its warts under a compatibility layer.

I imagine people who use these GHC internals are already used to constant breaking changes so this patch won't be too surprising for them.

Precisely my thoughts. The clients of ghc-heap API are likely to be exposed to much larger dangers than this.

[Bodigrim]

Instead I'm thinking about publishing a wherefrom-compat package to Hackage. This would give a consistent interface for 9.4, 9.6, and 9.8 (once it's released). Folks who want a stable interface can depend on that package. This would also help deal with the move from GHC.Stack.CCS to GHC.InfoProv in 9.6.
An entry in the migration guide could point users towards this.

That's a very nice idea.

---

Dear CLC members, let's vote on the proposal to move ClosureType from ghc-heap into a new module GHC.ClosureTypes in base and use it instead of Word in GHC.Stack.CloneStack.StackEntry.closureType and instead of String in GHC.InfoProv.InfoProv.ipDesc. See details at https://gitlab.haskell.org/ghc/ghc/-/merge_requests/9560.

This is a breaking change, but both modules are very new and virtually unused (see https://hackage-search.serokell.io/?q=import.*GHC.Stack.CloneStack and https://hackage-search.serokell.io/?q=import.*GHC.InfoProv). To mitigate the breakage the proposer is willing to develop a compatibility package.

@tomjaguarpaw @chshersh @mixphix @angerman @parsonsmatt @hasufell

---

+1 from me.

[chshersh]

+1

[tomjaguarpaw]

-1

---

because this doesn't satisfy the conditions in #122 (comment). Actually I don't see why it can't be in a ghc-* that base depends on, such as ghc-prim or a new ghc-* package.

Have I misunderstood something here? Are the things exported here expected to be stable? Or is there something else I have missed? If so then please correct me. I can change my vote if necessary. But at the moment I don't see why this GHC internal should leak from base. We already have too many such leaks. Let's not add more.

[TeofilC]

@tomjaguarpaw Forgot to reply to your comment before. I'd be happy to change the module to be called GHC.Internal.ClosureType (or something else with .Internal in the name). So long as GHC folks are happy with that as well

[Bodigrim]

@TeofilC there've been plenty of time to respond and amend before the vote, please do not invalidate the votes already cast by changing the proposal. We are volunteers and being wasteful with our limited resources is not encouraged. Nevertheless you do have a formal right to withdraw the proposal and submit a new one, if you wish.

That said, I did expect the new API to be stable and not to be broken on a whim in future. Is it not the case?

[TeofilC]

Very fair! Do ignore my comment about changing the module name.

Presumably GHC.ClosureType would need to be updated if a new closure type is added in the RTS (though looking at the history the only time the interface changed was when continuation objects were recently added). My understanding is that GHC.InfoProv will be stable.

[parsonsmatt]

+1

[mixphix]

-1

[angerman]

I--similar like @tomjaguarpaw probably--had hoped to hear from @bgamari on this as to how this ties in with the future plans. I will say I do prefer the better naming; it makes debugging and reading much easier. Yet we now replicate a data structure in base from a header in the rts. And this is in a non-automated fashion, looking at the MR, I do not see how they are kept in sync. We now risk that at some point the rts's header file might change, and base starts to diverge; now we do have a hard dependency on the rts from base already (base calls all kinds of things in the rts), but I'd still like to ensure that these do not diverge. Especially as the new module is exposed from base; in which case I also believe the ClosureType should have a more extensive haddock documentation. Someone is going to look at this, and eventually try to use it (which again raises what @tomjaguarpaw mentioned: is this a stable api?), so there needs to be some discussion around ClosureTypes in the Haddocks.

For now I'm -1 on this. However, if given

• clarity on stability
• checks that this stays synchronised between the rts and base
• good documentation on the newly exposed module
• compat path (as @Bodigrim mentioned @mpickering is willing to add)

I'd be +1.

[hasufell]

I also vote -1 for this specific instance and largely agree with @angerman

Dealing with Int in low-level API is trash, so this generally looks like an improvement if we can clear up the mentioned concerns.

So I'd be excited to vote on a follow up proposal.

[TeofilC]

Just to clarify, I'm the one committing to writing to a compat package not mpickering. He's the proposer, while I'm the one who wrote the GHC MR.

[TeofilC]

Thanks everyone for your thoughts!

I feel like the main concern is about the stability of this interface.
It's probably best to revisit this once we see how the chips fall with #145 and #146.

checks that this stays synchronised between the rts and base

There's already a checklist in ClosureTypes.h that has a list of places in the rts that need to be updated when adding/changing a closure type. Would your concern here be satisfied @angerman if I added an item to also update GHC.ClosureType? Unfortunately I don't have the capacity to implement something like an automated check that the C and the Haskell code line up.

good documentation on the newly exposed module

Would a short paragraph summarising what a closure type is (and the fact that it's an implementation detail of the RTS) and a link to https://gitlab.haskell.org/ghc/ghc/-/wikis/commentary/rts/storage/heap-objects suffice. Or did you have something more in mind @angerman ?

[bgamari]

@angerman, I had not given much consideration to the interaction between this and #145/#146 until your ping. Having reflected on this a bit, I would propose that, if #145 is accepted, the GHC.ClosureTypes and the closureType field of StackEntry should only be exported from ghc-base as they are both highly coupled to GHC's internal representation.

Another solution would be for base to export the closureType field and the ClosureType type itself, without its constructors. This would provide accept to, e.g., the Show and Eq instances of ClosureType while avoiding exposing anything outwardly unstable declarations in base.

My sense is that the former, more conservative option is preferable.

[Bodigrim]

It's probably best to revisit this once we see how the chips fall with #145 and #146.

Just to be clear, both #145 and #146 are in early stages and may easily take several months. I think there are good chances for a follow-up proposal, which adresses raised concerns, to be approved earlier.

---

Closing as declined (I assume @tomjaguarpaw remains unsatisfied). Thanks for your effort and discussion, @TeofilC.