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

Apply thread labels to internal worker threads #305

Closed
bgamari opened this issue Nov 14, 2024 · 12 comments
Closed

Apply thread labels to internal worker threads #305

bgamari opened this issue Nov 14, 2024 · 12 comments
Labels
approved Approved by CLC vote awaits-merge Approved, but MR is still unmerged

Comments

@bgamari
Copy link

bgamari commented Nov 14, 2024

Various functions in base are implemented via the use of auxiliary threads. Recently a user requested that GHC apply thread labels to these threads to make these threads clearly identifiable in the event log and listThreads output.

Specifically, I propose that the following functions' implementations be augmented with thread labels where appropriate:

  • Control.Concurrent.threadWaitRead
  • Control.Concurrent.threadWaitWrite
  • Control.Concurrent.threadWaitReadSTM
  • Control.Concurrent.threadWaitWriteSTM
  • System.Timeout.timeout
  • GHC.Conc.Signal.runHandlers

The proposed thread labels are static and consequently should have negligible impact on runtime.

Implementation: !13566

@tomjaguarpaw
Copy link
Member

How would this work with listThreads exactly? We have listThreads :: IO [ThreadId], and we can label a thread with labelThread (presumably overwriting any existing label). But I don't see a way of reading the label. How would one do that?

@bgamari
Copy link
Author

bgamari commented Nov 14, 2024

threadLabel :: ThreadId -> IO (Maybe String) exposed by GHC.Conc.Sync allows one to retrieve the label of a thread (IMHO these operations should be exposed from a more sensibly named module).

@bgamari
Copy link
Author

bgamari commented Dec 2, 2024

Gentle ping. From my perspective this proposal is ready for a vote.

@bgamari
Copy link
Author

bgamari commented Dec 10, 2024

Is there anything I can do to move this along? I would very much appreciate being able to get this off my desk.

@tomjaguarpaw
Copy link
Member

@Bodigrim, what's the next step?

@Bodigrim
Copy link
Collaborator

(Sorry, I'm massively overwhelmed for the past month or so)

Related discussion:

@tomjaguarpaw following the Reddit discussion, do you have any specific concerns about the proposal at hand? I understand that you are somewhat unhappy about the existence of listThreads at all, but labelling threads does not seem to make it materially worse.

@tomjaguarpaw
Copy link
Member

but labelling threads does not seem to make it materially worse.

I agree. I don't see the labels themselves as a problem. This proposal is fine by me.

@Bodigrim
Copy link
Collaborator

Dear CLC members, let's vote on the proposal to annotate internal worker threads in base with helpful labels as detailed in https://gitlab.haskell.org/ghc/ghc/-/merge_requests/13566/diffs. This is purely a quality-of-life change.

@tomjaguarpaw @velveteer @mixphix @hasufell @angerman @parsonsmatt


+1 from me.

@Bodigrim Bodigrim added the vote-in-progress The CLC vote is in progress label Dec 14, 2024
@parsonsmatt
Copy link

+1

2 similar comments
@tomjaguarpaw
Copy link
Member

+1

@hasufell
Copy link
Member

+1

@Bodigrim
Copy link
Collaborator

Thanks all, that's enough votes to approve.

@Bodigrim Bodigrim added approved Approved by CLC vote and removed vote-in-progress The CLC vote is in progress labels Dec 16, 2024
@Bodigrim Bodigrim added the awaits-merge Approved, but MR is still unmerged label Dec 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote awaits-merge Approved, but MR is still unmerged
Projects
None yet
Development

No branches or pull requests

5 participants