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

Refactoring block_for_gc #100

Merged
merged 6 commits into from
Sep 12, 2023
Merged

Conversation

udesou
Copy link
Contributor

@udesou udesou commented Sep 7, 2023

Currently, we keep calling into C multiple times to set up some variables before the thread stops for GC and after GC is done. This PR refactors the code in block_for_gc to delegate that to C, reusing most of the code in jl_gc_collect. That should help preventing errors in the future due to the mismatch between the implementations as it keeps things in a single place.

Should be merged with mmtk/julia#29.

@udesou udesou marked this pull request as ready for review September 7, 2023 06:18
@udesou udesou requested a review from qinsoon September 7, 2023 06:18
Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

LGTM, though we may be able to remove some duplicated code.

// when executing finalizers do not let another thread do GC (set a variable such that while that variable is true, no GC can be done)
int8_t set_gc_initial_state(void* ptls_raw)
// based on jl_gc_collect from gc.c
JL_DLLEXPORT void jl_gc_prepare_to_collect(void)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this function just duplicates jl_gc_collect. Have you considered moving the function to gc-common.c, and letting Julia and MMTk each implement a different _jl_gc_collect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that triggering GC manually in Julia is done via a call to jl_gc_collect (https://github.com/mmtk/julia/blob/83796a7d38878faa9afdb94ce84b6da00035b336/base/gcutils.jl#L129). For MMTk, we want that function to call into mmtk_handle_user_collection_request as it currently does. That's why I did the duplication, with jl_gc_prepare_to_collect doing the actual preparation that is done in jl_gc_collect from gc.c.
We could potentially reuse jl_gc_collect for what I'm doing by passing a different argument (eg. -1), but we'd still have the duplication - the code for Julia in gc.c and the code for MMTk in mmtk-gc.c.

@udesou udesou merged commit ad945ae into mmtk:master Sep 12, 2023
9 checks passed
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.

2 participants