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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reorganize gc.c to prepare for thread-safe variant, with thread context hack. #10735

Merged
merged 1 commit into from
Apr 6, 2015

Conversation

ArchRobison
Copy link
Contributor

This is a continuation of #10724. It's a new PR because Github does not allow reopening a pull request after a forced push, which I learned the hard way 馃槮

The goal of this PR is still the same: modify gc.c in ways to prepare for making it thread-safe later. All changes in this PR are intended to be cosmetic and not impact functionality or performance.

The improvement over #10724 is the use of macros FOR_CURRENT_HEAP and FOR_EACH_HEAP for establishing thread context. In the thread-safe version, these will look something like:

#define FOR_CURRENT_HEAP {jl_thread_heap_t* current_heap = jl_thread_heap;
#define END }
#define FOR_EACH_HEAP for( int t=jl_n_threads; --t>=0; )  { jl_thread_heap_t* current_heap = jl_all_heaps[t];
/*}*/
#define HEAP(x) (current_heap->x)

In the current PR, these macros devolve in a way that retains the current logic, albeit with a dummy variable current_heap that is used to check that the macros are being used where needed (albeit no checking for correct use.) Any modern compiler will optimize away the dummy variable.

The other change over #10724 is that I put the finalizer stuff back in its original position. Because one thread can set up a finalizer, but another thread can ask for it to be executed, correct implementation is going to require more than just per-thread lists, so that's a battle for another day.

@ArchRobison ArchRobison added the domain:multithreading Base.Threads and related functionality label Apr 3, 2015
else {
ndel++;
}
} while ((n < l-ndel) && SWAP_wr(lst[n],lst[n+ndel]));
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

The tabs here may make the build fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was wondering what the extra green meant. I guessed tabs, but I couldn't find them in my working copy. I'll poke around to figure out how to eradicate them.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This out to work:

perl -i -ple 's/\t/    /g'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is convincing Github that the tabs are not there. When I check out my branch, src/gc.c has no tabs.

Copy link
Member

Choose a reason for hiding this comment

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

Are those actually hard tabs? I believe github also displays it like this for soft tabs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is a soft tab?

Copy link
Member

Choose a reason for hiding this comment

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

Spaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's puzzling me is why it's displaying those lines differently than many others that I also indented by 4 spaces.

Copy link
Member

Choose a reason for hiding this comment

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

GitHub can't always detect changes within a line. E.g. below where you added a static declaration, the same green box exists.

@ArchRobison ArchRobison force-pushed the adr/gcpolish branch 3 times, most recently from a7f98c5 to 0097775 Compare April 3, 2015 21:09
@ArchRobison
Copy link
Contributor Author

Patch is 100% tab-free ingredients. @carnaval - okay to commit?

@carnaval
Copy link
Contributor

carnaval commented Apr 6, 2015

LGTM. The macros don't even look that hacky to me, it's pretty much all we need inside gc as far as I can tell. This + a real implementation of the thread contexts + a barrier around the collection and we should have something somewhat working.

ArchRobison pushed a commit that referenced this pull request Apr 6, 2015
Reorganize gc.c to prepare for thread-safe variant, with thread context hack.
@ArchRobison ArchRobison merged commit 26a01f2 into JuliaLang:master Apr 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:multithreading Base.Threads and related functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants