[blitz.mod] Debug builds secretly reactivate the GC even once GCSuspend()-ed

[GWRon]

While investigating why "for loops" require so much longer in debug builds (1000000 loops release <1ms, debug 3000ms) I have seen that
blitz.mod/debugger_mt.stdio.bmx does a lot of GCSuspend() and GCResume().

Function OnDebugEnterScope( scope:Int Ptr)',inst:Byte Ptr )
	Local dbgState:TDbgState = GetDbgState()
	GCSuspend
...
	GCResume	
End Function

Function OnDebugLeaveScope()
	Local dbgState:TDbgState = GetDbgState()
	GCSuspend
...
	GCResume	
End Function

Function OnDebugPushExState()

	Local dbgState:TDbgState = GetDbgState()
	GCSuspend
...
And so on.

as GCSuspend simply calls GC_disable(); and GCResume simply calls GC_enable(); each of these debugger-functions directly affects the GC.

These now bears two issues:

• if your application code for some reason disabled the GC it will be overridden right in the next code statement when doing debug builds (as the next enter-scope will already GCResume at the end)
• issue [blitz.mod] Debug builds: GCSuspend/GCResume for each "local block" #316

Edit:
PS: Just keeping the GCSuspend() and GCResume() calls in the Enter/Leave-Functions (disabling the rest of their function logic) cuts down the execution time from 3000 to 700ms for me. Means the GCResume/Suspend alone does 75% of the additional runtime.

[HurryStarfish]

if your application code for some reason disabled the GC it will be overridden right in the next code statement when doing debug builds (as the next enter-scope will already GCResume at the end)

According to gc.h:

GC_disable() and GC_enable() calls nest. Garbage collection is enabled if the number of calls to both functions is equal.

Does this not work as documented?

[GWRon]

Thanks for that hint... it should do what it says in the documentation.

bdwgc's misc.c contains this:

GC_API void GC_CALL GC_enable(void)
{
    LOCK();
    GC_ASSERT(GC_dont_gc != 0); /* ensure no counter underflow */
    GC_dont_gc--;
    if (!GC_dont_gc && GC_heapsize > GC_heapsize_on_gc_disable)
      WARN("Heap grown by %" WARN_PRIuPTR " KiB while GC was disabled\n",
           (GC_heapsize - GC_heapsize_on_gc_disable) >> 10);
    UNLOCK();
}

GC_API void GC_CALL GC_disable(void)
{
    LOCK();
    if (!GC_dont_gc)
      GC_heapsize_on_gc_disable = GC_heapsize;
    GC_dont_gc++;
    UNLOCK();
}

GC_API int GC_CALL GC_is_disabled(void)
{
    return GC_dont_gc != 0;
}

So that "75% of the additional runtime" are surely connected to the locking and unlocking of a mutex.

And checking the above code there is no reason for the "nesting" not working as it should.

But ... if it "nests", is the logic in our OnDebugEnterScope etc (see first post in the issue here) then really working?
Let's check.
"blitz_gc.c" (blitz.mod) does this (bbGCSuspend is directly linked to blitzmax' GCSuspend() etc):

void bbGCSuspend(){
	GC_disable();
}

void bbGCResume(){
	GC_enable();
}

This means that each time you call GCSuspend() it calls GC_disable() and thus increasing that GC_dont_gc-counter of bwdgc. And GCResume() calls GC_enable() which lowers GC_dont_gc down to 0 (not less).

This means to provoke an issue one could do this:

'start with an active GC ...

GCResume() 'GC_dont_gc is now 0
GCResume() 'GC_dont_gc is still 0
GCSuspend() 'GC_dont_gc is now 1
GCSuspend() 'GC_dont_gc is still 2

This exposes that the "nesting" only works if done in the correct order (suspend only if running).

Calls like "OnDebugEnterScope()" will "suspend" and "resume" in the correct order - and this is OK as long as nothing expects the "GC" to be actually resumed afterwards (it will still be suspended if it was suspended before).

So it looks OK to me.

[GWRon]

What do you think about adding a "GCIsSuspended()" thing - so a getter interpreting GC_dont_gc (reading could possibly not require a lock?).
Or alternatively GCSuspend() and GCResume() could return the "nesting level".

Why? Because this would allow to skip "GCSuspend()" and "GCResume()" calls if there won't be a change -> and thus avoiding to lock a mutex up to 2 times. It depends on how "costly" it is to retrieve an atomic value if the gc is suspended or not (already).

[HurryStarfish]

reading could possibly not require a lock?

Reading still needs a lock, otherwise you might not get the latest value if it's been changed by a different thread.

this would allow to skip "GCSuspend()" and "GCResume()" calls if there won't be a change

Then what if another thread calls GCResume() in the meantime?

[GWRon]

Reading still needs a lock

In our case it should just lead to more locks than needed (so up to 2) ("I need to gcsuspend, nobody else did before" because it missed, that someone else suspended too meanwhile).
So in that case it does more than needed, but not less.

Then what if another thread calls GCResume() in the meantime?

These function all do "suspend ...something else ... resume". So as long as you store the "have to wrap my 'something else' in suspend + resume" flag at the beginning of your function, it does not matter what "others" do meanwhile - as others have to take care of their "suspend + resume" on their own.

think of the code being similar to:

If not GCSuspended()
  GCSuspend()
  DoMyThing()
  GCResume()
Else
  DoMyThing()
EndIf

(of course you would do differently). It does not matter if others also GCSuspend/Resume() somewhere.

It is not right that it does NOT matter ... it matters. Because the current code is already not acting threadsafe then.

See this function:

Function OnDebugEnterScope( scope:Int Ptr)',inst:Byte Ptr )
	Local dbgState:TDbgState = GetDbgState()
	GCSuspend
...
	GCResume	
End Function

(must not be this one - just some function doing "GCSuspend" + "GCResume").

thread 1 calls OnDebugEnterScope now ... was just running GCSuspend.
thread 2 already entered their OnDebugEnterScope (or some other function ...see above) earlier - and is now already calling GCResume.

Voila GC is now set to continue working while thread 1 assumes it is safe to do things now (it just suspended the GC - right?).

So either it does not matter if others do their gcsuspend()/gcresume() somewhere - or the current implementation is not guaranteeing thread safety (did not check when or how these debug-functions are called, but someone doing similar stuff in their code has to take care then).

The older debugger part (blitz.mod/debugger.stdio.bmx) only called above functions from the main thread - the current implemention (blitz.mod/debugger_mt.stdio.bmx) does not care in which thread it runs. Dunno how threadsafe it is at the end.

[HurryStarfish]

thread 1 calls OnDebugEnterScope now ... was just running GCSuspend.
thread 2 already entered their OnDebugEnterScope (or some other function ...see above) earlier - and is now already calling GCResume.

Voila GC is now set to continue working while thread 1 assumes it is safe to do things now (it just suspended the GC - right?).

No, because thread 2 also called GCSuspend before. So in total you've now had two calls to GCSuspend and one call to GCResume - the GC is still suspended. But if thread 1 had skipped its GCSuspend call, then the GC could run again as soon as thread 2 calls GCResume, while thread 1 is in the middle of doing its thing.

[GWRon]

Ah yes you are right ... I somehow already forgot about the "counter" and just had an "on/off" toggle in my mind. Too much computer time today.

Will close now. If you can imagine of a way to avoid "two locks" somehow (maybe something like the atomicadd() stuff ... ) then feel free to write to me at discord..

Most people won't run 1.000.000 for loops in debug - but some might do, and optimizing their "debug build experience" isn't the worst we could do.