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

Bring a few upstream GC patches into our fork #169

Merged
merged 5 commits into from
Jul 25, 2024

Conversation

d-netto
Copy link
Member

@d-netto d-netto commented Jul 22, 2024

PR Description

Should be NFCs or code simplifications.

JuliaLang#54936 is the only one which does more significant changes, but we already assessed its performance in the open-source benchmarks and internally as well.

Checklist

Requirements for merging:

d-netto and others added 5 commits July 22, 2024 14:32
Encapsulates all relevant GC thread-local-state into a separate
structure.

Motivation is that MMTk will have its own version of GC
thread-local-state, so doesn't need all of the Julia GC TLS.

In the future, folks who would be using MMTk would be setting a
pre-processor flag which would lead to either the stock Julia GC TLS or
MMTk's GC TLS to be included in `julia_threads.h`.

I.e., we would have something like:

```C
jl_gc_mmtk_tls_states mmtk_gc_tls;
jl_gc_tls_states gc_tls;
```
Simplifies the layout of the doubly linked list of big objects to make
it a bit more canonical: let's just store a pointer to the previous
element, instead of storing a "pointer to the next element of the
previous element". This should make the implementation a bit easier to
understand without incurring any memory overhead.

I ran the serial and multithreaded benchmarks from GCBenchmarks and this
seems fairly close to performance neutral on my machine. We also ran our
internal benchmarks on it at RAI and it looks fine from a correctness
and performance point of view.

---------

Co-authored-by: Kiran Pamnany <[email protected]>
Third-party GCs (e.g. MMTk) will probably have their own function to
spawn GC threads.
@github-actions github-actions bot added port-to-v1.10 This change should apply to Julia v1.10 builds port-to-master This change should apply to all future Julia builds labels Jul 22, 2024
@d-netto d-netto removed the port-to-master This change should apply to all future Julia builds label Jul 22, 2024
@d-netto d-netto requested a review from kpamnany July 24, 2024 13:29
@kpamnany
Copy link
Collaborator

Let's run benchmarks on this full commit set?

@d-netto
Copy link
Member Author

d-netto commented Jul 24, 2024

Let's run benchmarks on this full commit set?

Yes.

@d-netto
Copy link
Member Author

d-netto commented Jul 25, 2024

Ran the benchmarks in https://github.com/RelationalAI/raicode/pull/20439.

Copy link
Collaborator

@kpamnany kpamnany left a comment

Choose a reason for hiding this comment

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

❤️

@d-netto d-netto merged commit 5f234f0 into v1.10.2+RAI Jul 25, 2024
5 checks passed
@d-netto d-netto deleted the dcn-upstream-gc-patches branch July 25, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
port-to-v1.10 This change should apply to Julia v1.10 builds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants