Skip to content

Commit

Permalink
fix many module imports causing GC to pull the rug on our module inst…
Browse files Browse the repository at this point in the history 
…ance
  • Loading branch information
@ruby0x1
ruby0x1 committed Jul 11, 2020
1 parent 433fbc4 commit d432b03
Showing 1 changed file with 11 additions and 0 deletions.
11 changes: 11 additions & 0 deletions src/vm/wren_vm.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -446,10 +446,21 @@ static ObjClosure* compileInModule(WrenVM* vm, Value name, const char* source,
{
module = wrenNewModule(vm, AS_STRING(name));

// It's possible for the map set call below to resize the modules map,
// and trigger a GC while doing so. When this happens and there are enough
// objects and modules, it will collect the module we've just created.
// wrenPushRoot isn't an option when there are enough modules to exhaust
// WREN_MAX_TEMP_ROOTS, however handles are also not collected by GC.
// Note that this only affects the module until it's in the map, because
// then it has a reference to it, and won't be collected.
WrenHandle* moduleHandle = wrenMakeHandle(vm, OBJ_VAL(module));

This comment has been minimized.

Sign in to view

Copy link
@mhermier

mhermier Jul 11, 2020

Contributor

This solution sounds really sub-optimal.

Can you elaborate more on why you think using wrenPushRoot is not an option ?

The scope of the push/pop is really centered around wrenMapSet and I don't see how it can sanely lead to an exhaust of roots. But I rember WREN_MAX_TEMP_ROOTS was defined to be the minimal amount required so the engine works. I would prefered WREN_MAX_TEMP_ROOTS to be some power of 2 than that random 5. So please revert use wrenPushRoot and increase WREN_MAX_TEMP_ROOTS.

This comment has been minimized.

Sign in to view

Copy link
@ruby0x1

ruby0x1 Jul 11, 2020

Author Member

I need to test it again in some of my big projects to validate this since it's been a while. It's possible the behavior is different and I've since made it more granular, it was a few years now when I made this fix.

To add context, this code was a direct result of the vm asserting when I was importing modules normally in a big project. Having any upper bound is unreasonable imo, as it means that a big project can't exist. To set the upper bound really high, since this is a preallocated array, you pay 8 * N up front (x64). At the time I used WREN_MAX_TEMP_ROOTS set to 8192 because of this and used push/pop, but recently while solving something else I realized that a handle is effectively just a GC root. So IMO I don't find this exact solution to be a problem, but it's possible that it isn't necessary to use a handle.

I'll test more before changing it however, since I have many existing projects that would fail if the original conditions are still there.

This comment has been minimized.

Sign in to view

Copy link
@mhermier

mhermier via email Jul 11, 2020

Contributor

// Store it in the VM's module registry so we don't load the same module
// multiple times.
wrenMapSet(vm, vm->modules, name, OBJ_VAL(module));

wrenReleaseHandle(vm, moduleHandle);

// Implicitly import the core module.
ObjModule* coreModule = getModule(vm, NULL_VAL);
for (int i = 0; i < coreModule->variables.count; i++)
Expand Down

0 comments on commit d432b03

Please sign in to comment.