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

Reorganize gc.c to prepare for thread-safe variant. #10724

Closed
wants to merge 1 commit into from

Conversation

ArchRobison
Copy link
Contributor

This is a nudge in the direction of making gc.c thread-safe. Changes are intended to be cosmetic (albeit in the ugly direction) with no functional or performance impact.

  • Most of the changes just reorder the source file so that some global variables are put together. These variables will become fields of a struct in the threads-safe version.
  • Some accesses to the aforementioned variables are wrapped in a macro HEAP(var) that expands to (var). These accesses will become field accesses in the thread-safe version, at which time HEAP will look like #define HEAP(var) (jl_thread_heap->var). Unwrapped accesses are ones that are going to require more work, typically because they involve iterating over multiple instances in the thread-safe version.
  • Some functions and variables have been marked staticsince they were unreferenced outside gc.c.
  • Variable perm_marked was deleted because it was unreferenced.
  • Part of jl_gc_init() was split out. It's the part that will do per-thread initializations in the thread-safe version.

@carnaval
Copy link
Contributor

carnaval commented Apr 2, 2015

Had a quick look, looks good.
Just to clarify, we are still doing single thread gc right ? (for now)
So some of those HEAP(...) accesses would actually have to go through every threads' structures ?

@ViralBShah ViralBShah added the multithreading Base.Threads and related functionality label Apr 2, 2015
@ArchRobison
Copy link
Contributor Author

@carnaval, it would be good if you took a look. Please check where I wrote:

// Global variables that become thread-local in thread-safe version.

and see if I nabbed all the suspects.

@ArchRobison
Copy link
Contributor Author

Right, it's still single-threaded. There's still a lot of changes to implement the per-thread stuff, but this change should make the other ones easier to understand since there won't be a need to move large blocks of source code.

@carnaval
Copy link
Contributor

carnaval commented Apr 2, 2015

Ok. About the patch I don't see anything left out in the global list. Essentially everything that is touched during allocation must be thread local (for correctness and perf) and the rest is only used during collection so can stay global. Does that seems a fair rule ?

@ArchRobison
Copy link
Contributor Author

That rule is my intent.

@ArchRobison
Copy link
Contributor Author

I've removed the HEAP notation. I found what I think is a syntactically cleaner hack to deal with the per-heap accesses. I'm going to temporarily close this PR and reopen after I've tried my hack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants