Skip to content
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.

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

document push .. pop pattern in clox #19

Closed
zxul767 opened this issue Nov 16, 2022 · 3 comments · Fixed by #34
Closed

document push .. pop pattern in clox #19

zxul767 opened this issue Nov 16, 2022 · 3 comments · Fixed by #34
Labels
documentation Improvements or additions to documentation p0

Comments

@zxul767
Copy link
Owner

zxul767 commented Nov 16, 2022

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 zxul767 added the documentation Improvements or additions to documentation label Nov 17, 2022
@zxul767
Copy link
Owner Author

zxul767 commented Dec 6, 2022

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 zxul767 added the p0 label Dec 7, 2022
@zxul767
Copy link
Owner Author

zxul767 commented Dec 7, 2022

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 zxul767 mentioned this issue Dec 9, 2022
@zxul767
Copy link
Owner Author

zxul767 commented Dec 14, 2022

#34 will fix this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation p0
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant