[MLton] cvs commit: critical sections during thread switch

[Stephen Weeks]

> But we have a stronger invariant for the current thread; we can shrink
> stacks on the heap down until stack->reserved == stack->used (modulo
> alignment), but we can't do that for the current thread's stack.  For the
> current thread, we can't drop reserved below
> 
>   stack->used + stackSlop - topFrameSize(stack)
> 
> which is precisely what stackNeedsReserved is computing. 

True.

> (And, therefore, was preserving the mutatorStackInvariant for stacks
> on the heap.)

At the start of doGC, the mutator thread may not satisfy
mutatorStackInvariant (the stack may need to grow).  Neither need any
other stack on the heap.  So, there's no invariant to preserve. 

> So, it seems to me that for stack resizing, we ought to keep
> stackNeedsReserved as is -- there's no sense in breaking the
> mutatorStackInvariant on heap stacks if we can preserve it.

It does cost space in every paused thread, which could be bad if
maxFrameSize is large.

How about treating the current stack as a special case in forward?
For all the paused stacks, either leave the stack alone or shrink the
stack while keeping mutatorStackInvariant or (used <= reserved) true,
depending on which we decide is better.  For the current stack, if we
are not in a minor gc and the stack needs to grow, we could grow it
right there in forward.  That would be nice because it would avoid
recopying the stack later in growStack after the GC.  It would also
avoid wasted space in the old generation for the dead stack.  Also, it
would let us request less space in doGC than we are now.  Instead of
requesting

	stackBytes (s, 2 * s->currentThread->stack->reserved)

we only need to request

	stackBytes (s, 2 * s->currentThread->stack->reserved)
	- stackBytes (s, s->currentThread->stack->reserved)

That is, we only need the additional space that forward will use to
to skip over reserved space for the stack.

I do see the comment in the code about not being able to grow the
stack from within forward, but it seems to me that if doGC requests
the right amount of space, then it will be there for forward.  Maybe
that comment is vestigal.

> The mutatorStackInvariant is not true for the initial stack created by
> newWorld.  So, we can skip the mutatorInvariant assert at the end of
> GC_init, or push the mutatorStackInvariant check only back into the loaded
> world case; or leave the stackIsEmpty test.
> 
> I'm in favor of dropping the stackIsEmpty and pushing the
> mutatorStackInvariant check into the loaded world case, leaving an
> invariant(s) check at the join point.

Sounds good.  I guess you could put a mutatorFrontierInvariant at the
join point as well.

> Also, I discovered that the mutatorFrontierInvariant is not maintained;
> or, more accurately, the current thread's bytesNeeded is not maintained at
> every entry into the runtime. 

Right.  Looking at limit-check.fun, I see that a limit check may be
inserted at a CReturn if the function mayGC and not ensuresBytesFree.
That is, upon return from such a function, the mutator assumes nothing
about the frontier.  So, there is no point in setting bytesNeeded for
such functions.  Also, there is no point in setting bytesNeeded for a
function that won't switch threads, since the calling thread will
never get paused at that point.

Here are some C functions and their attributes.

			mayGC	maySwitchThreads 	ensuresBytesFree
			-----	----------------	----------------
GC_arrayAllocate	true	false			true
GC_gc			true	true or false		true
GC_copyCurrentThread	true	false			false
GC_copyThread		true	false			false
Thread_returnToC	true	true			false
Thread_switchTo		true	true			true

This shows the full spectrum of possibilities.  The maySwitchThreads
property for GC_gc depends on whether the program uses threads or not.
Based on the above reasoning, I would expect GC_arrayAllocate,
GC_copyCurrentThread, GC_copyThread, and Thread_returnToC to not
bother with bytesNeeded, while I would expect GC_gc and
Thread_switchTo to bother.  And indeed, GC_gc and Thread_switchTo are
the only places in the runtime that assign to
s->currentThread->bytesNeeded.

> In particular, I was seeing cases where invoking GC_copyThread used
> up enough of the heap that the frontier invariant was invalidated,
> since the current thread's bytesNeeded went down by the space
> consumed by the new thread.

Makes sense, since the mutator doesn't assume that the invariant is
true after GC_copyThread.  I guess the right thing is to put
mutatorFrontierInvariant only where the mutator assumes it is true
:-), that is at the end of GC_gc and whenever switching to a thread,
i.e. at the end of GC_switchToThread.

> I also note that since GC_switchToThread doesn't change the bytesNeeded of
> the currentThread, if the current thread was saved and then resumed, then
> it will attempt to get as much heap as it needed at it's last GC point,
> which may be more than it needs after the switch point.

Thread_switchTo in basis/Thread.c does set bytesNeeded before invoking
GC_switchToThread, so I don't see a problem there.  However, it's
confusing having thread switching behavior split across these two
functions and I don't see any reason for it.  Maybe we could fold
Thead_switchTo into GC_switchToThread?