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 to be considered before adding MMTk #55608

Merged
merged 11 commits into from
Oct 19, 2024

Conversation

udesou
Copy link
Contributor

@udesou udesou commented Aug 28, 2024

This PR contains some refactoring of common functions that were moved to gc-common.c and should be shared between MMTk and Julia's stock GC.

@nsajko nsajko added the GC Garbage collector label Aug 28, 2024
src/staticdata.c Outdated Show resolved Hide resolved
@d-netto
Copy link
Member

d-netto commented Aug 29, 2024

@udesou, a comment before the review: I think rebase might be preferable over merging master to sync a branch, since it avoids interleaving your commits with the upstream ones, and keeps the log "cleaner" (e.g. all of your commits after HEAD).

@udesou
Copy link
Contributor Author

udesou commented Aug 30, 2024

@udesou, a comment before the review: I think rebase might be preferable over merging master to sync a branch, since it avoids interleaving your commits with the upstream ones, and keeps the log "cleaner" (e.g. all of your commits after HEAD).

Sounds good. I was just clicking the button on GitHub without changing anything but I think I did it right this time.

@udesou udesou marked this pull request as ready for review September 1, 2024 23:48
@udesou udesou changed the title WIP: Refactoring to be considered before adding MMTk Refactoring to be considered before adding MMTk Sep 1, 2024
src/stackwalk.c Outdated Show resolved Hide resolved
src/julia_internal.h Outdated Show resolved Hide resolved
src/julia.h Outdated Show resolved Hide resolved
src/gc-stock.h Outdated Show resolved Hide resolved
src/gc-stock.c Show resolved Hide resolved
src/gc-stock.c Outdated Show resolved Hide resolved
src/gc-stacks.c Show resolved Hide resolved
src/gc-stacks.c Show resolved Hide resolved
src/gc-debug.c Outdated Show resolved Hide resolved
src/gc-common.c Outdated Show resolved Hide resolved
@fingolfin
Copy link
Contributor

@udesou if you don't mind, could you perhaps click "Resolve conversation" for comments that you already addressed? That would make it a tad easier to follow what's going on here. Thank you, and thank you for this work!

src/gc-common.c Outdated Show resolved Hide resolved
src/julia_internal.h Outdated Show resolved Hide resolved
@udesou udesou force-pushed the refactor-pre-mmtk branch 2 times, most recently from 5805609 to 173b0db Compare September 30, 2024 08:24
@udesou udesou force-pushed the refactor-pre-mmtk branch 2 times, most recently from 127cc78 to bc9e77e Compare October 8, 2024 09:13
@udesou udesou requested a review from d-netto October 8, 2024 09:13
Copy link
Member

@d-netto d-netto left a comment

Choose a reason for hiding this comment

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

Please address the gcext failure on CI before merging.

@d-netto
Copy link
Member

d-netto commented Oct 8, 2024

@fingolfin: I thought we moved gcext out of "Allow Fail" and into a regular test that needs to pass for CI to be green?

If not, perhaps we should consider doing it given that we'll be working on a bunch of GC PR's in the next weeks and it might be useful to have a very explicit indication (e.g. red CI) of whether we broke gcext.

@udesou
Copy link
Contributor Author

udesou commented Oct 9, 2024

Please address the gcext failure on CI before merging.

@d-netto I think it should be fixed now, however, some unrelated things seem to be failing (by looking at CI results from the latest commit in the Julia repo). If it's really unrelated, then we should be good to go, I guess.

Copy link
Member

@d-netto d-netto left a comment

Choose a reason for hiding this comment

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

Reading through it LGTM, but, as usual, let's wait for CI to merge...

@udesou udesou force-pushed the refactor-pre-mmtk branch 2 times, most recently from 29cce04 to 3662c12 Compare October 17, 2024 04:48
@d-netto
Copy link
Member

d-netto commented Oct 17, 2024

@udesou. CI looks fine. Could you fix the merge conflict in src/gc-debug.c?

@d-netto d-netto merged commit 877de98 into JuliaLang:master Oct 19, 2024
7 checks passed
KristofferC pushed a commit that referenced this pull request Oct 21, 2024
This PR contains some refactoring of common functions that were moved to
`gc-common.c` and should be shared between MMTk and Julia's stock GC.
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.

5 participants