Skip to content

Conversation

@wenkokke
Copy link
Collaborator

@wenkokke wenkokke commented Dec 9, 2024

No description provided.

@wenkokke
Copy link
Collaborator Author

wenkokke commented Dec 9, 2024

@dcoutts This uses performBlockingMajorGC when available and uses your trick otherwise. I've provided a bit of explanation, and removed the first assert, since asserting twice seems superfluous. It'd give you fail-fast behaviour, but in the common case you hope to not fail.

@wenkokke
Copy link
Collaborator Author

wenkokke commented Dec 9, 2024

@dcoutts This still requires yield in order to pass the test suite, even when using performBlockingMajorGC.

I'll look into what it actually does. In the meantime, this is WIP.

@wenkokke wenkokke marked this pull request as draft December 9, 2024 19:42
@wenkokke
Copy link
Collaborator Author

wenkokke commented Dec 9, 2024

From cursory reading of GHC source, performBlockingMajorGC appears mostly to be the correct alternative to use for compatibility with non-moving concurrent GC. So we should probably keep your hack and combine it with preferring performBlockingMajorGC if it’s available?

Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this whole thing could be a lot simpler.

How about:

import qualified System.Mem

and then

-- Later GHC versions have 'performBlockingMajorGC' which does a full GC even
-- when using the concurrent incremental collector. This is what we want to ensure
-- all forgotten refs are found.
#if MIN_VERSION_base(4,20,0)
performBlockingMajorGC = System.Mem.performBlockingMajorGC
#else
performBlockingMajorGC = System.Mem.performMajorGC
#endif

and otherwise in checkForgottenRefs, keep it the same as originally but replace performMajorGC with performBlockingMajorGC. And the comment you added is good too.

But there's nothing magic about performBlockingMajorGC that means we don't need yield or that means we should do it once vs twice. The performBlockingMajorGC is just the same as performMajorGC in older GHC versions except that it provides the same guarantee for the incremental GC as the non-incremental: that it scans the whole heap.

Copy link
Collaborator

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now. Please squash and merge.

@wenkokke wenkokke force-pushed the wenkokke/rc-blocking-gc branch from 9662e61 to d8d5b19 Compare December 10, 2024 13:15
@wenkokke wenkokke marked this pull request as ready for review December 10, 2024 13:16
@wenkokke wenkokke enabled auto-merge December 10, 2024 13:16
@wenkokke wenkokke force-pushed the wenkokke/rc-blocking-gc branch from 7451eb9 to 544385e Compare December 10, 2024 13:53
@dcoutts
Copy link
Collaborator

dcoutts commented Dec 10, 2024

Unclear what's up with stylish-haskell.

@wenkokke wenkokke force-pushed the wenkokke/rc-blocking-gc branch from 544385e to f1a646c Compare December 10, 2024 15:17
@wenkokke wenkokke changed the title refcount: If available, use performBlockingMajorGC refcount: Use performBlockingMajorGC if available Dec 10, 2024
@wenkokke
Copy link
Collaborator Author

Unclear what's up with stylish-haskell.

stylish-haskell really messed up owing to the fact that I didn't run it.

@wenkokke wenkokke added this pull request to the merge queue Dec 10, 2024
Merged via the queue into main with commit 5f4e933 Dec 10, 2024
27 checks passed
@wenkokke wenkokke deleted the wenkokke/rc-blocking-gc branch December 10, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants