From 2c88e1949774dc04b3df0fa94c2ef09e4da0367b Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Sat, 17 Mar 2018 09:33:33 -0700 Subject: [PATCH] Compile imports to closures, not fibers. This is simpler and marginally faster. We don't need the overhead of fibers since you can't have long or recursive import chains anyway. More importantly, this makes the behavior more well-defined when you do things like yield from an imported module. (Not that you should do that, but if you do, it shouldn't do weird things.) --- src/optional/wren_opt_meta.c | 12 +- src/optional/wren_opt_meta.wren | 6 +- src/optional/wren_opt_meta.wren.inc | 6 +- src/vm/wren_vm.c | 152 +++++++++--------- src/vm/wren_vm.h | 15 +- test/core/fiber/yield_from_import.wren | 14 ++ test/core/fiber/yield_from_import_module.wren | 4 + 7 files changed, 105 insertions(+), 104 deletions(-) create mode 100644 test/core/fiber/yield_from_import.wren create mode 100644 test/core/fiber/yield_from_import_module.wren diff --git a/src/optional/wren_opt_meta.c b/src/optional/wren_opt_meta.c index 32b8f35f..504543be 100644 --- a/src/optional/wren_opt_meta.c +++ b/src/optional/wren_opt_meta.c @@ -14,18 +14,18 @@ void metaCompile(WrenVM* vm) bool printErrors = wrenGetSlotBool(vm, 3); // TODO: Allow passing in module? - ObjFiber* fiber = wrenCompileSource(vm, "main", source, - isExpression, printErrors); - + ObjClosure* closure = wrenCompileSource(vm, "main", source, + isExpression, printErrors); + // Return the result. We can't use the public API for this since we have a - // bare ObjFiber*. - if (fiber == NULL) + // bare ObjClosure*. + if (closure == NULL) { vm->apiStack[0] = NULL_VAL; } else { - vm->apiStack[0] = OBJ_VAL(fiber); + vm->apiStack[0] = OBJ_VAL(closure); } } diff --git a/src/optional/wren_opt_meta.wren b/src/optional/wren_opt_meta.wren index 6871dc9f..d6fdecf4 100644 --- a/src/optional/wren_opt_meta.wren +++ b/src/optional/wren_opt_meta.wren @@ -10,11 +10,11 @@ class Meta { static eval(source) { if (!(source is String)) Fiber.abort("Source code must be a string.") - var fiber = compile_(source, false, false) + var closure = compile_(source, false, false) // TODO: Include compile errors. - if (fiber == null) Fiber.abort("Could not compile source code.") + if (closure == null) Fiber.abort("Could not compile source code.") - fiber.call() + closure.call() } static compileExpression(source) { diff --git a/src/optional/wren_opt_meta.wren.inc b/src/optional/wren_opt_meta.wren.inc index cfcf77dc..a372e10b 100644 --- a/src/optional/wren_opt_meta.wren.inc +++ b/src/optional/wren_opt_meta.wren.inc @@ -12,11 +12,11 @@ static const char* metaModuleSource = " static eval(source) {\n" " if (!(source is String)) Fiber.abort(\"Source code must be a string.\")\n" "\n" -" var fiber = compile_(source, false, false)\n" +" var closure = compile_(source, false, false)\n" " // TODO: Include compile errors.\n" -" if (fiber == null) Fiber.abort(\"Could not compile source code.\")\n" +" if (closure == null) Fiber.abort(\"Could not compile source code.\")\n" "\n" -" fiber.call()\n" +" closure.call()\n" " }\n" "\n" " static compileExpression(source) {\n" diff --git a/src/vm/wren_vm.c b/src/vm/wren_vm.c index 4fa1a388..712dfd81 100644 --- a/src/vm/wren_vm.c +++ b/src/vm/wren_vm.c @@ -474,8 +474,8 @@ static ObjModule* getModule(WrenVM* vm, Value name) return !IS_UNDEFINED(moduleValue) ? AS_MODULE(moduleValue) : NULL; } -static ObjFiber* compileInModule(WrenVM* vm, Value name, const char* source, - bool isExpression, bool printErrors) +static ObjClosure* compileInModule(WrenVM* vm, Value name, const char* source, + bool isExpression, bool printErrors) { // See if the module has already been loaded. ObjModule* module = getModule(vm, name); @@ -505,19 +505,12 @@ static ObjFiber* compileInModule(WrenVM* vm, Value name, const char* source, return NULL; } - wrenPushRoot(vm, (Obj*)fn); - // Functions are always wrapped in closures. + wrenPushRoot(vm, (Obj*)fn); ObjClosure* closure = wrenNewClosure(vm, fn); - wrenPushRoot(vm, (Obj*)closure); - - ObjFiber* moduleFiber = wrenNewFiber(vm, closure); - - wrenPopRoot(vm); // closure. wrenPopRoot(vm); // fn. - // Return the fiber that executes the module. - return moduleFiber; + return closure; } // Verifies that [superclassValue] is a valid object to inherit from. That @@ -700,6 +693,60 @@ void wrenFinalizeForeign(WrenVM* vm, ObjForeign* foreign) finalizer(foreign->data); } +Value wrenImportModule(WrenVM* vm, Value name) +{ + // If the module is already loaded, we don't need to do anything. + if (!IS_UNDEFINED(wrenMapGet(vm->modules, name))) return NULL_VAL; + + const char* source = NULL; + bool allocatedSource = true; + + // Let the host try to provide the module. + if (vm->config.loadModuleFn != NULL) + { + source = vm->config.loadModuleFn(vm, AS_CSTRING(name)); + } + + // If the host didn't provide it, see if it's a built in optional module. + if (source == NULL) + { + ObjString* nameString = AS_STRING(name); +#if WREN_OPT_META + if (strcmp(nameString->value, "meta") == 0) source = wrenMetaSource(); +#endif +#if WREN_OPT_RANDOM + if (strcmp(nameString->value, "random") == 0) source = wrenRandomSource(); +#endif + + // TODO: Should we give the host the ability to provide strings that don't + // need to be freed? + allocatedSource = false; + } + + if (source == NULL) + { + vm->fiber->error = wrenStringFormat(vm, "Could not load module '@'.", name); + return NULL_VAL; + } + + ObjClosure* moduleClosure = compileInModule(vm, name, source, false, true); + + // Modules loaded by the host are expected to be dynamically allocated with + // ownership given to the VM, which will free it. The built in optional + // modules are constant strings which don't need to be freed. + if (allocatedSource) DEALLOCATE(vm, (char*)source); + + if (moduleClosure == NULL) + { + vm->fiber->error = wrenStringFormat(vm, + "Could not compile module '@'.", name); + return NULL_VAL; + } + + // Return the closure that executes the module. + return OBJ_VAL(moduleClosure); +} + // The main bytecode interpreter loop. This is where the magic happens. It is // also, as you can imagine, highly performance critical. Returns `true` if the // fiber completed without error. @@ -1208,17 +1255,12 @@ static WrenInterpretResult runInterpreter(WrenVM* vm, register ObjFiber* fiber) Value result = wrenImportModule(vm, name); if (!IS_NULL(fiber->error)) RUNTIME_ERROR(); - // If we get a fiber, switch to it to execute the module body. - if (IS_FIBER(result)) + // If we get a closure, call it to execute the module body. + if (IS_CLOSURE(result)) { STORE_FRAME(); - - // Return to this module when that one is done. - AS_FIBER(result)->caller = fiber; - - // Switch to the module's fiber. - fiber = AS_FIBER(result); - vm->fiber = fiber; + ObjClosure* closure = AS_CLOSURE(result); + callFunction(vm, fiber, closure, 1); LOAD_FRAME(); } @@ -1369,13 +1411,17 @@ WrenInterpretResult wrenInterpret(WrenVM* vm, const char* source) WrenInterpretResult wrenInterpretInModule(WrenVM* vm, const char* module, const char* source) { - ObjFiber* fiber = wrenCompileSource(vm, module, source, false, true); - if (fiber == NULL) return WREN_RESULT_COMPILE_ERROR; + ObjClosure* closure = wrenCompileSource(vm, module, source, false, true); + if (closure == NULL) return WREN_RESULT_COMPILE_ERROR; + wrenPushRoot(vm, (Obj*)closure); + ObjFiber* fiber = wrenNewFiber(vm, closure); + wrenPopRoot(vm); // closure. + return runInterpreter(vm, fiber); } -ObjFiber* wrenCompileSource(WrenVM* vm, const char* module, const char* source, +ObjClosure* wrenCompileSource(WrenVM* vm, const char* module, const char* source, bool isExpression, bool printErrors) { Value nameValue = NULL_VAL; @@ -1385,65 +1431,11 @@ ObjFiber* wrenCompileSource(WrenVM* vm, const char* module, const char* source, wrenPushRoot(vm, AS_OBJ(nameValue)); } - ObjFiber* fiber = compileInModule(vm, nameValue, source, - isExpression, printErrors); + ObjClosure* closure = compileInModule(vm, nameValue, source, + isExpression, printErrors); if (module != NULL) wrenPopRoot(vm); // nameValue. - return fiber; -} - -Value wrenImportModule(WrenVM* vm, Value name) -{ - // If the module is already loaded, we don't need to do anything. - if (!IS_UNDEFINED(wrenMapGet(vm->modules, name))) return NULL_VAL; - - const char* source = NULL; - bool allocatedSource = true; - - // Let the host try to provide the module. - if (vm->config.loadModuleFn != NULL) - { - source = vm->config.loadModuleFn(vm, AS_CSTRING(name)); - } - - // If the host didn't provide it, see if it's a built in optional module. - if (source == NULL) - { - ObjString* nameString = AS_STRING(name); -#if WREN_OPT_META - if (strcmp(nameString->value, "meta") == 0) source = wrenMetaSource(); -#endif -#if WREN_OPT_RANDOM - if (strcmp(nameString->value, "random") == 0) source = wrenRandomSource(); -#endif - - // TODO: Should we give the host the ability to provide strings that don't - // need to be freed? - allocatedSource = false; - } - - if (source == NULL) - { - vm->fiber->error = wrenStringFormat(vm, "Could not load module '@'.", name); - return NULL_VAL; - } - - ObjFiber* moduleFiber = compileInModule(vm, name, source, false, true); - - // Modules loaded by the host are expected to be dynamically allocated with - // ownership given to the VM, which will free it. The built in optional - // modules are constant strings which don't need to be freed. - if (allocatedSource) DEALLOCATE(vm, (char*)source); - - if (moduleFiber == NULL) - { - vm->fiber->error = wrenStringFormat(vm, - "Could not compile module '@'.", name); - return NULL_VAL; - } - - // Return the fiber that executes the module. - return OBJ_VAL(moduleFiber); + return closure; } Value wrenGetModuleVariable(WrenVM* vm, Value moduleName, Value variableName) diff --git a/src/vm/wren_vm.h b/src/vm/wren_vm.h index 97d75d79..617f62b0 100644 --- a/src/vm/wren_vm.h +++ b/src/vm/wren_vm.h @@ -139,18 +139,9 @@ WrenInterpretResult wrenInterpretInModule(WrenVM* vm, const char* module, // execute it. // // Returns NULL if a compile error occurred. -ObjFiber* wrenCompileSource(WrenVM* vm, const char* module, const char* source, - bool isExpression, bool printErrors); - -// Imports the module with [name], a string. -// -// If the module has already been imported (or is already in the middle of -// being imported, in the case of a circular import), returns null. Otherwise, -// returns a new fiber that will execute the module's code. That fiber should -// be called before any variables are loaded from the module. -// -// If the module could not be found, sets an error in the current fiber. -Value wrenImportModule(WrenVM* vm, Value name); +ObjClosure* wrenCompileSource(WrenVM* vm, const char* module, + const char* source, bool isExpression, + bool printErrors); // Looks up a variable from a previously-loaded module. // diff --git a/test/core/fiber/yield_from_import.wren b/test/core/fiber/yield_from_import.wren new file mode 100644 index 00000000..dbb0e77d --- /dev/null +++ b/test/core/fiber/yield_from_import.wren @@ -0,0 +1,14 @@ +var fiber = Fiber.new { + System.print("fiber 1") + + import "yield_from_import_module" + + System.print("fiber 2") +} + +fiber.call() // expect: fiber 1 + // expect: module 1 +System.print("main 1") // expect: main 1 +fiber.call() // expect: module 2 + // expect: fiber 2 +System.print("main 2") // expect: main 2 diff --git a/test/core/fiber/yield_from_import_module.wren b/test/core/fiber/yield_from_import_module.wren new file mode 100644 index 00000000..0bb88398 --- /dev/null +++ b/test/core/fiber/yield_from_import_module.wren @@ -0,0 +1,4 @@ +// nontest +System.print("module 1") +Fiber.yield() +System.print("module 2")