zxul767 / lox

An interpreter for the Lox language
1 stars 0 forks source link

document `push .. pop` pattern in `clox` #19

Closed zxul767 closed 1 year ago

zxul767 commented 1 year ago

There are various places in clox where the push .. pop pattern shows up. This pattern is used so that the garbage collector will not free memory it shouldn't by temporarily placing a newly created object on the value stack, and then popping it once it's stored in another place where it belongs (e.g., the interned strings table, or the global variables table).

This is typically necessary when the newly created object is being pushed to a container which can trigger a memory allocation right before the object is stored in the container (a potential time for the garbage collector to kick in).

zxul767 commented 1 year ago

after thinking about this more, i think it's a bad idea to keep using this pattern. it is prone to errors and results in tedious and verbose code.

an alternative is what i've called "the nursery pattern", which is the idea of having some means to keep track of newly allocated objects for a specific duration. for example, instead of:

push_value(OBJECT_VAL(string__copy(name, (int)strlen(name), vm)), vm);
push_value(OBJECT_VAL(native_function__new(function, vm)), vm);
table__set(&vm->global_vars, AS_STRING(vm->value_stack[0]), vm->value_stack[1]);
pop_value(vm);
pop_value(vm);

we might do:

memory__open_object_nursery(vm);
// e.g., `vm->nursery_start = vm->objects`

ObjectString* function_name = string__copy(name, (int)strlen(name), vm));
Value function = OBJECT_VAL(native_function__new(function, vm)));
// during a GC run (which might happen in `table__set`), we expose objects 
// between `vm->nursery_start` and `vm->objects` as roots so they are not 
// incorrectly disposed
table__set(&vm->global_vars, function_name, function);

memory__close_object_nursery(vm);
// e.g., `vm->nursery_start = NULL`

which is both more performant (no more pushing and popping, nor unnecessary value creation), and less error prone.

zxul767 commented 1 year ago

marked as p0 due to the potential, subtle bugs that not implementing this and continuing to rely on the push/pop pattern can bring

zxul767 commented 1 year ago

https://github.com/zxul767/lox/pull/34 will fix this...