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

(re-factor) gc: remove redundant since_sweep field #49195

Merged
merged 1 commit into from
May 4, 2023

Conversation

topolarity
Copy link
Member

This field is at all times 0 or identical to allocd, so this change removes it in preference of gc_num.allocd.

Hopefully this makes the GC metrics a tiny bit easier to interpret for newcomers (like myself).

This field is at all times 0 or identical to allocd, so this change
removes it in preference of `gc_num.allocd`. Hopefully this makes the
GC metrics a bit easier to interpret for newcomers.
@gbaraldi gbaraldi requested a review from d-netto March 30, 2023 13:35
@d-netto
Copy link
Member

d-netto commented Mar 30, 2023

My concern is whether people could be using gc_num.since_sweep. Probably unlikely since it's GC internal, but still.

@topolarity
Copy link
Member Author

My concern is whether people could be using gc_num.since_sweep. Probably unlikely since it's GC internal, but still.

I suppose if we want to avoid the breaking change we can provide a wrapper in timing.jl to fill in the removed field?

@KristofferC
Copy link
Member

JuliaHub shows no uses in packages at least.

@giordano giordano added the GC Garbage collector label Mar 30, 2023
@d-netto
Copy link
Member

d-netto commented Apr 13, 2023

Seems like we use since_sweep internally at RAI but are in the process of updating the GC metrics we export, which would eliminate the use of such metric among other things.

I believe removing this metric would be fine from our perspective.

Tagging @NHDaly for more input on that.

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.

4 participants