Skip to content

Test and fix garbage collector #45

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

Draft
ferdnyc wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Test and fix garbage collector #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3c57953
RE fixes for modern compilers
ferdnyc Sep 27, 2025
acf55d2
Add debugging to GC collection
ferdnyc Sep 27, 2025
51d70ff
Add basic GC test
ferdnyc Sep 27, 2025
beb65a8
Define DEBUG_PRINTF macro in debug mode
ferdnyc Sep 27, 2025
f5d69fd
Garbage collection: Fix full GC runs
ferdnyc Sep 27, 2025
eeeacf6
Update GC test, still doesn't really test GC
ferdnyc Sep 27, 2025
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
8 changes: 4 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ TESTS_PY_FILES=$(wildcard tests/*.py)
# rule to make objects for static linkage
.objs/%.o : %.c
@mkdir -p $(dir $@)
$(CC) $(CFLAGS) -g -O0 -I . -c -o $@ $<
$(CC) $(CFLAGS) -DNDEBUG -g -O0 -I . -c -o $@ $<
Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025

Choose a reason for hiding this comment

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

Added this to the non-debug compile commands, since it apparently wasn't being defined. (I think because of the -g.)

Copy link

@rofl0r rofl0r Sep 27, 2025

Choose a reason for hiding this comment

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

-g -O0 looks like a debug compile command to me

Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025

Choose a reason for hiding this comment

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

@rofl0r In truth, it's identical to the actual debug compile command a few lines down, except that it doesn't define TPVM_DEBUG. I'm guessing turning on optimization in the non-debug builds was something left for after the code was shored up a bit better.


.dbgobjs/%.o : %.c
@mkdir -p $(dir $@)
Expand All @@ -53,7 +53,7 @@ TESTS_PY_FILES=$(wildcard tests/*.py)
# rule to make objects for dynamic linkage
.dynobjs/%.o : %.c
@mkdir -p $(dir $@)
$(CC) $(CFLAGS) -fPIC -g -O0 -I . -c -o $@ $<
$(CC) $(CFLAGS) -DNDEBUG -fPIC -g -O0 -I . -c -o $@ $<


all: tpy tpvm
Expand Down Expand Up @@ -97,15 +97,15 @@ GENERATED_SOURCE_FILES+=tinypy/tp_opcodes.h

# tpvm only takes compiled byte codes (.tpc files)
tpvm : $(VMLIB_FILES:%.c=.objs/tinypy/%.o) .objs/tinypy/vmmain.o modules/modules.a
$(CC) $(CFLAGS) -g -O0 -o $@ $^ -lm
$(CC) $(CFLAGS) -DNDEBUG -g -O0 -o $@ $^ -lm
#
# tpvm only takes compiled byte codes (.tpc files)
tpvm-dbg : $(VMLIB_FILES:%.c=.dbgobjs/tinypy/%.o) .dbgobjs/tinypy/vmmain.o modules/modules.a
$(CC) $(CFLAGS) -g -O0 -o $@ $^ -lm

# tpy takes .py files
tpy : $(TPLIB_FILES:%.c=.objs/tinypy/%.o) .objs/tinypy/tpmain.o modules/modules.a
$(CC) $(CFLAGS) -o $@ $^ -lm
$(CC) $(CFLAGS) -DNDEBUG -o $@ $^ -lm

# tpy takes .py files
tpy-dbg : $(TPLIB_FILES:%.c=.dbgobjs/tinypy/%.o) .dbgobjs/tinypy/tpmain.o modules/modules.a
Expand Down
10 changes: 5 additions & 5 deletions modules/re/regexpr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,11 +137,11 @@ void re_compile_fastmap(regexp_t compiled);
extern int re_syntax;
extern unsigned char re_syntax_table[256];
void re_compile_initialize();
int re_set_syntax();
char *re_compile_pattern();
int re_match();
int re_search();
void re_compile_fastmap();
int re_set_syntax(int);
char *re_compile_pattern(unsigned char*, int, regexp_t);
int re_match(regexp_t, unsigned char*, int, int, regexp_registers_t);
int re_search(regexp_t, unsigned char*, int, int, int, regexp_registers_t);
void re_compile_fastmap(regexp_t);
Comment on lines +140 to +144
Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025

Choose a reason for hiding this comment

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

This is what I meant about needing to update the code for modern GCCs. Around GCC 11 or 12, declarations like the ones that were there previously became a compile error.

Copy link

@rofl0r rofl0r Sep 27, 2025

Choose a reason for hiding this comment

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

fyi that's a matter of default standard, GCC 15 defaults to -std=gnu23 (which makes this an error), whereas earlier standards accepted it, so adding e.g. -std=gnu99 would fix it too. (note that using -std=c99 is more complicated since it enables a "strict C without extensions" profile on most platforms, which makes common POSIX or XSI extensions unavailable).

Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025

Choose a reason for hiding this comment

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

Good to know, tho I figure the declarations are better with the argument types provided anyway, so it's not really worth fighting the standard. 😀

Copy link

@rofl0r rofl0r Sep 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

indeed, having the full prototype here is an improvement. in other cases where one just wants to compile a piece of software that shipped before GCC 15 materialized, it's usually a lot less hassle to just add -std=gnu99 to CFLAGS than trying to patch the software. and with compilers moving as fast and recklessly like in the past few years it's generally a good idea to just hardcode the expected C/C++ standard one wants to use in the build environment, so it will build even in a decade into the future.

Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025

Choose a reason for hiding this comment

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

so it will build even in a decade into the future.

I admire your optimism! 😁


#endif /* HAVE_PROTOTYPES */

Expand Down
41 changes: 41 additions & 0 deletions tests/test_gc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
from tinypy.runtime.testing import UnitTest

class Create:
def __init__(self, **args):
self.__dict__.update(args)

def _create_with_a(a):
return Create(a=a)
create_with_a = staticmethod(_create_with_a)

def _create_with_b(b):
return Create(b=b)
create_with_b = staticmethod(_create_with_b)


class GcTest(UnitTest):
objA = None
objB = None
scrap_objects = []

def test_create(self):
self.objA = Create.create_with_a(3)
assert self.objA.a == 3
self.objB = Create.create_with_b(3)
assert self.objB.b == 3

def test_delete(self):
assert self.objA is not None
assert self.objB is not None
for i in range(20):
new_obj = Create(a=i)
self.scrap_objects.append(new_obj)
del self.objA
del self.objB
#assert self.objA is None
#assert self.objB is None


t = GcTest()

t.run()
28 changes: 22 additions & 6 deletions tinypy/tp_gc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@
#define TP_GC_TRACE 0
#define TP_GC_ASSERT_LISTS_ARE_DISJOINT 0 /* Assert no white object is on the black list. Very slow. */

/* Maximum number of objects to collect per partial GC run. */
#define TP_GC_STEP_MAX 8

/* Macros for min/max, borrowed from glibc sys/param.h */
#ifndef MAX
#define MIN(a,b) (((a)<(b))?(a):(b))
#define MAX(a,b) (((a)>(b))?(a):(b))
#endif

/* tp_grey: ensure an object to the grey list, if the object is already
* marked grey, then do nothing. */
void tp_grey(TP, tp_obj v) {
Expand Down Expand Up @@ -196,8 +205,14 @@ void tp_collect(TP) {
tp->black->len = 0;
}

void tp_mark(TP, int max) {
while (tp->grey->len && max > 0) {
void tp_mark(TP, int gc_max) {
int target_len = gc_max > 0 ? MAX(0, tp->grey->len - gc_max) : 0;
DEBUG_PRINTF(
"GC: Coloring objects, %d out of %d to mark\n",
tp->grey->len - target_len, tp->grey->len
);

while (tp->grey->len > target_len) {
Comment on lines +208 to +215
Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

So, this fixes the garbage collection. The DEBUG_PRINTF()s show that it's now running and doing its thing, instead of doing nothing like before. (Although it's interesting that, at least when running the tests, ONLY full GC runs were ever triggered — which made the issue even worse, because partial runs would've actually collected some objects. But there were never any partial runs.)

Even having fixed it, though, I'm kind of at a loss for how to actually test it. The "tests" I added to tests/test_gc.py really don't test the GC at all, in the sense of confirming that it's doing what it should be doing. They merely create and delete some objects to trigger the DEBUG_PRINTF() output, so that the GC operation can be manually inspected. But there are no tests for it in the code.

I'm really not sure how to write them, either. Without weakref support (which I don't think tinypy has?), it seems like a difficult thing to actually confirm that an unreferenced object has been deleted. Especially by a GC that, unlike the Python one, has no code-accessible API.

Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025

Choose a reason for hiding this comment

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

Also, now that I've read more of the code I don't actually like this method.

The problem comes up (potentially) when complex nested objects are GC'd. If a container is moved from the grey list to the black list, its children are placed on the grey list. Which means the length of the grey list can actually INCREASE as the loop is running, which would cause it to mark more than gc_max objects each run.

So, making tp->grey->len the loop control variable is a bad idea, and I'll need to add an explicit loop iteration counter back in.

Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025

Choose a reason for hiding this comment

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

And, indeed, now that I actually have incremental steps working (see https://github.com/rainwoodman/tinypy/pull/45/files#r2384173153), the logs regularly show things like:

GC: Incremental step, marking up to 8 objects
GC: Coloring objects, 8 out of 31 to mark
GC: Marking complete, 105 objects remain on grey list
GC: Incremental step, marking up to 8 objects
GC: Coloring objects, 8 out of 105 to mark
GC: Marking complete, 101 objects remain on grey list
GC: Incremental step, marking up to 8 objects
GC: Coloring objects, 8 out of 101 to mark
GC: Marking complete, 97 objects remain on grey list

My previous algorithm would've screwed up "8 out of 31 to mark => marking complete, 105 remain" pretty spectacularly.

tp_obj v;
/* pick a grey object */
v = tpd_list_pop(tp, tp->grey, tp->grey->len-1, "_tp_gcinc");
Expand All @@ -214,8 +229,8 @@ void tp_mark(TP, int max) {

/* put children to grey. */
tp_follow(tp,v);
if(max > 0) max--;
}
DEBUG_PRINTF("GC complete, %d items remaining\n", tp->grey->len);
}

void tp_gc_dump(TP, tpd_list * l, int name, int mark) {
Expand All @@ -242,10 +257,12 @@ void tp_gc_dump(TP, tpd_list * l, int name, int mark) {

void tp_gc_run(TP, int full) {
if (full || tp->gcmax == 0 || (tp->steps % tp->gcmax == 0)) {
DEBUG_PRINTF("Running full GC\n");
tp_mark(tp, -1);
} else {
/* mark 2 items from the grey list every step */
tp_mark(tp, 8);
/* mark a fixed number of items from the grey list every step */
DEBUG_PRINTF("Running partial GC of up to %d items\n", TP_GC_STEP_MAX);
tp_mark(tp, TP_GC_STEP_MAX);
}

/* grey list is empty, we can run a collection */
Expand Down Expand Up @@ -286,4 +303,3 @@ void tp_gc_deinit(TP) {


/**/

8 changes: 8 additions & 0 deletions tinypy/tp_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,11 @@ tp_inline static tpd_frame * tp_get_cur_frame(TP) { return tp_get_frame(tp, tp->

/* Detect unintended size changes. Update as needed. */
STATIC_ASSERT(sizeof(tpd_code) == 4, "size of tpd_code must be 4");

#ifndef DEBUG_PRINTF
#ifdef NDEBUG
#define DEBUG_PRINTF(...) {0;}
#else
#define DEBUG_PRINTF(...) fprintf(stderr, __VA_ARGS__)
#endif
#endif
Comment on lines +28 to +35
Copy link
Author

@ferdnyc ferdnyc Sep 27, 2025

Choose a reason for hiding this comment

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

I added this to tp_internal to replace the hand-coded logic I was using in tp_gc.c — it should be portable to any C99 compiler, __VA_ARGS__ is part of the standard now. And printing to stderr means the messages can still be suppressed if needed.