Skip to content

Julia GC: fix stack marking #3199

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.

Sign up for GitHub

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

Merged
ThomasBreuer merged 1 commit into from
Jan 21, 2019
Merged

Julia GC: fix stack marking #3199

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/julia_gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,6 +364,7 @@ static jl_datatype_t * datatype_mptr;
static jl_datatype_t * datatype_bag;
static jl_datatype_t * datatype_largebag;
static UInt StackAlignBags;
static Bag * GapStackBottom;
static jl_ptls_t JuliaTLS, SaveTLS;
static size_t max_pool_obj_size;
static size_t bigval_startoffset;
Expand Down Expand Up @@ -527,6 +528,9 @@ void GapRootScanner(int full)
// current_task != root_task.
char * stackend = (char *)jl_task_stack_buffer(task, &size, &tid);
stackend += size;
if (JuliaTLS->tid == 0 && JuliaTLS->root_task == task) {
stackend = (char *)GapStackBottom;
Copy link
Member

@fingolfin fingolfin Jan 20, 2019

Choose a reason for hiding this comment

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

Hmm, but note that this only will work right if GAP is the main process, loading Julia later. If, however, one is starting Julia and from there loading GAP, then GapStackBottom may not be at the base of the stack frame.

This is closely related to (part of) the discussion on issue #3089 ; see also PR #3096

Copy link
Contributor Author

@rbehrends rbehrends Jan 20, 2019

Choose a reason for hiding this comment

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

Yes, and GapTaskScanner() may also need an update. That said, it should properly be handled so that Julia gets to know the proper end of the stack for task switching.

Copy link
Contributor

@embray embray Jan 22, 2019

Choose a reason for hiding this comment

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

ISTM there needs to be a SetStackBottomBags (as in #3096) for Julia GC as well.

Copy link
Member

@fingolfin fingolfin Jan 22, 2019

Choose a reason for hiding this comment

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

Possible, but this code is very much in flux, so it doesn't really matter at this stage.

}

// allow installing a custom marking function. This is used for
// integrating GAP (possibly linked as a shared library) with other code
Expand Down Expand Up @@ -589,6 +593,13 @@ static void PreGCHook(int full)
// a thread-local variable.
SaveTLS = JuliaTLS;
JuliaTLS = jl_get_ptls_states();
// This is the same code as in VarsBeforeCollectBags() for GASMAN.
// It is necessary because ASS_LVAR() and related functionality
// does not call CHANGED_BAG() for performance reasons. CHANGED_BAG()
// is only called when the current lvars bag is being changed. Thus,
// we have to add a write barrier at the start of the GC, too.
if (STATE(CurrLVars))
CHANGED_BAG(STATE(CurrLVars));
Copy link
Member

@fingolfin fingolfin Jan 20, 2019

Choose a reason for hiding this comment

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

It's OK to have this in here for now, to get a quick fix; but on the long run, I prefer to fix this properly, by providing a generic "pre-gc hook" in both GASMAN (which already has one) and Julia GC, and then setting that to VarsBeforeCollectBags. Also, this "optimization" for lvars needs to be documented in a better place. But neither of these are change request for this PR; me or somebody else can take care of it in a future PR.

Copy link
Contributor Author

@rbehrends rbehrends Jan 20, 2019

Choose a reason for hiding this comment

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

I agree. My goal here was to get the fix out ASAP. As the code is identical to VarsBeforeCollectBags(), this should be unified.

/* information at the beginning of garbage collections */
SyMsgsBags(full, 0, 0);
memset(MarkCache, 0, sizeof(MarkCache));
Expand Down Expand Up @@ -638,6 +649,7 @@ static uintptr_t JMarkBag(jl_ptls_t ptls, jl_value_t * obj)
void InitBags(UInt initial_size, Bag * stack_bottom, UInt stack_align)
{
// HOOK: initialization happens here.
GapStackBottom = stack_bottom;
for (UInt i = 0; i < NUM_TYPES; i++)
TabMarkFuncBags[i] = MarkAllSubBags;
// These callbacks need to be set before initialization so
Expand Down