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 code that is specific to stock GC write barriers #57237

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

udesou
Copy link
Contributor

@udesou udesou commented Feb 3, 2025

This PR moves some code responsible for implementing write barriers around. In codegen, the code for rewriting write barriers is put into each gc-specific file (src/llvm-late-gc-lowering-*.cpp). For the runtime, the write barrier functions are lifted from julia.h into the new gc-wb-*.h files.

@udesou udesou added the GC Garbage collector label Feb 3, 2025
@udesou udesou force-pushed the refactor/write-barriers branch 3 times, most recently from 1ff7739 to 1ce72aa Compare February 3, 2025 03:36
Copy link
Member

@vchuravy vchuravy left a comment

Choose a reason for hiding this comment

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

Do we know yet how the write barriers for mmtk will look like?

The GCs share a tag layout?

I would prefer doing this code movement when we have a functional implementaion and not a "TODO implement these". That would also include tests for the new lowering in https://github.com/JuliaLang/julia/tree/master/test/llvmpasses

@udesou udesou force-pushed the refactor/write-barriers branch from 1ce72aa to 19eb356 Compare February 3, 2025 22:30
@udesou
Copy link
Contributor Author

udesou commented Feb 3, 2025

Do we know yet how the write barriers for mmtk will look like?
I would prefer doing this code movement when we have a functional implementaion and not a "TODO implement these". That would also include tests for the new lowering in https://github.com/JuliaLang/julia/tree/master/test/llvmpasses

I agree that it's not ideal to have only the stubs for the MMTk implementation. This branch currently has the final implementation of sticky working: https://github.com/udesou/julia/tree/upstream-ready/sticky.
But what I've been doing is to try to create smaller PRs that encapsulate these refactorings before adding the code that actually implements MMTk's sticky (generational) collector. As for the tests, that's a great reminder. I think we should definitely add tests, but perhaps it makes more sense to add them in a separate PR (the one in which we add sticky immix?)

The GCs share a tag layout?

No, the log bit is currently implemented as part of a side metadata maintained by MMTk. We can potentially look into reusing Julia's tag layout but I think the code movement in this PR is actually orthogonal to it. Any other GC that we'd eventually plug in with a different layout would have to reimplement the barriers accordingly. So I think it makes sense that the code for implementing and lowering the write barriers should live in each gc-specific file.

@vchuravy
Copy link
Member

vchuravy commented Feb 4, 2025

No, the log bit is currently implemented as part of a side metadata maintained by MMTk.

Huh, okay. That means we will need quite some more code-modifications in areas where we are currently directly modifying the GC bits?

But what I've been doing is to try to create smaller PRs that encapsulate these refactorings

I love small refactorings, but they must make the code clearer on their own. Currently, without knowing the other side of this code, it's difficult to tell if this refactoring is beneficial or if a different strategy would be better.

As for the tests, that's a great reminder. I think we should definitely add tests, but perhaps it makes more sense to add them in a separate PR (the one in which we add sticky immix?)

It's good to start with tests for the code as is. Then you can test everything and show that once you actually add the implementation what the emitted code will look like.

@d-netto
Copy link
Member

d-netto commented Feb 4, 2025

The general idea here looks fine to me (have some minor comments which I will post shortly).

I believe that we proposed this refactor in the GC interface document already -- https://docs.google.com/document/d/1v0jtSrIpdEDNOxj5S9g1jPqSpuAkNWhr_T8ToFC9RLI/edit?usp=drivesdk.

The pattern we will follow to ensure that we inline these short functions in the
runtime is to conditionally #include in src/julia.h a header containing the write-barrier
implementation of Julia's stock GC or the implementation of the third-party GC.

@d-netto
Copy link
Member

d-netto commented Feb 4, 2025

I love small refactorings, but they must make the code clearer on their own. Currently, without knowing the other side of this code, it's difficult to tell if this refactoring is beneficial or if a different strategy would be better.

Could you clarify these concerns about code clarity @vchuravy?

Asking because I'd expect the code structure to remain fairly similar after we merge the generational MMTk and implement proper write barriers.

I.e. I think the call-sites of these functions will remain the same, though we will change some of them (e.g. jl_gc_wb) from no-ops to something actually functional.

@d-netto
Copy link
Member

d-netto commented Feb 4, 2025

I.e. I think the call-sites of these functions will remain the same, though we will change some of them (e.g. jl_gc_wb) from no-ops to something actually functional.

If that's the kind of refactor we will do, then I'm not sure how much more clarity we will be able to get by squashing this into a larger PR that introduces the generational MMTk.

@udesou udesou force-pushed the refactor/write-barriers branch from 378b069 to 509fc67 Compare February 4, 2025 22:01
@udesou udesou force-pushed the refactor/write-barriers branch from 509fc67 to 0d7268a Compare February 6, 2025 00:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GC Garbage collector
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants