From f7fddd9793c0593f7cfb8cbba95adeee75f7fe35 Mon Sep 17 00:00:00 2001 From: landerlyoung Date: Sat, 11 Dec 2021 18:09:23 +0800 Subject: [PATCH 1/4] don't do assert on QjsEnging creation, use exception for better debug. --- backend/QuickJs/QjsEngine.cc | 12 +++++++----- test/cmake/TestEnv.cmake | 24 ++++++++++++------------ 2 files changed, 19 insertions(+), 17 deletions(-) diff --git a/backend/QuickJs/QjsEngine.cc b/backend/QuickJs/QjsEngine.cc index cd9fd9ee..e71a6e7d 100644 --- a/backend/QuickJs/QjsEngine.cc +++ b/backend/QuickJs/QjsEngine.cc @@ -82,13 +82,15 @@ QjsEngine::QjsEngine(std::shared_ptr queue, const QjsFactor : queue_(queue ? std::move(queue) : std::make_shared()) { if (factory) { std::tie(runtime_, context_) = factory(); - assert(runtime_); - assert(context_); } else { runtime_ = JS_NewRuntime(); - assert(runtime_); - context_ = JS_NewContext(runtime_); - assert(context_); + if (runtime_) { + context_ = JS_NewContext(runtime_); + } + } + + if (!runtime_ || !context_) { + throw std::logic_error("QjsEngine: runtime or context is nullptr"); } initEngineResource(); diff --git a/test/cmake/TestEnv.cmake b/test/cmake/TestEnv.cmake index b3e700ba..8e366316 100644 --- a/test/cmake/TestEnv.cmake +++ b/test/cmake/TestEnv.cmake @@ -70,8 +70,8 @@ if (${SCRIPTX_BACKEND} STREQUAL V8) set(DEVOPS_LIBS_LIBPATH "${SCRIPTX_TEST_LIBS}/linux64/v8/libv8_monolith.a" CACHE STRING "" FORCE) - set(DEVOPS_LIBS_MARCO - V8_COMPRESS_POINTERS + set(DEVOPS_LIBS_MARCO + V8_COMPRESS_POINTERS CACHE STRING "" FORCE) elseif (WIN32) set(DEVOPS_LIBS_INCLUDE @@ -105,20 +105,20 @@ elseif (${SCRIPTX_BACKEND} STREQUAL JavaScriptCore) CACHE STRING "" FORCE) elseif (CMAKE_SYSTEM_NAME STREQUAL "Linux") set(DEVOPS_LIBS_INCLUDE - "${SCRIPTX_TEST_LIBS}/linux64/jsc/Headers" + "${SCRIPTX_TEST_LIBS}/linux64/jsc/Headers" CACHE STRING "" FORCE) set(DEVOPS_LIBS_LIBPATH - #"-Wl,--start-group" + #"-Wl,--start-group" "${SCRIPTX_TEST_LIBS}/linux64/jsc/libJavaScriptCore.a" - "${SCRIPTX_TEST_LIBS}/linux64/jsc/libWTF.a" - "${SCRIPTX_TEST_LIBS}/linux64/jsc/libbmalloc.a" - "dl" - "icudata" - "icui18n" - "icuuc" - "atomic" - #"-Wl,--end-group" + "${SCRIPTX_TEST_LIBS}/linux64/jsc/libWTF.a" + "${SCRIPTX_TEST_LIBS}/linux64/jsc/libbmalloc.a" + "dl" + "icudata" + "icui18n" + "icuuc" + "atomic" + #"-Wl,--end-group" CACHE STRING "" FORCE) elseif (WIN32) set(DEVOPS_LIBS_INCLUDE From 06fddf9f054bd38def8e4e3608dccc475656ca53 Mon Sep 17 00:00:00 2001 From: landerlyoung Date: Sat, 11 Dec 2021 18:29:38 +0800 Subject: [PATCH 2/4] EngineScope pass the previous engine pointer to EngineScopeImpl to eliminate another currentScope call inside the impl. --- backend/JavaScriptCore/JscScope.cc | 8 +++----- backend/JavaScriptCore/JscScope.h | 8 ++++---- backend/Lua/LuaScope.cc | 4 ++-- backend/Lua/trait/TraitScope.h | 2 +- backend/QuickJs/QjsScope.cc | 4 ++-- backend/QuickJs/trait/TraitScope.h | 2 +- backend/Template/trait/TraitScope.h | 2 +- backend/V8/V8Scope.cc | 2 +- backend/V8/V8Scope.h | 2 +- backend/WebAssembly/WasmScope.cc | 2 +- backend/WebAssembly/trait/TraitScope.h | 2 +- src/Scope.cc | 10 +++++----- src/Scope.h | 2 +- 13 files changed, 24 insertions(+), 26 deletions(-) diff --git a/backend/JavaScriptCore/JscScope.cc b/backend/JavaScriptCore/JscScope.cc index db136046..82717095 100644 --- a/backend/JavaScriptCore/JscScope.cc +++ b/backend/JavaScriptCore/JscScope.cc @@ -21,13 +21,11 @@ namespace script::jsc_backend { -JscEngineScope::JscEngineScope(JscEngine& engine) - : unlockPrevious_(&engine), lockGuard_(*engine.virtualMachineLock_) {} +JscEngineScope::JscEngineScope(JscEngine& engine, JscEngine* previous) + : unlockPrevious_(&engine, previous), lockGuard_(*engine.virtualMachineLock_) {} void JscEngineScope::UnlockPrevious_Ctor(JscEngine* currentEngine, - JscEngineScope::UnlockPrevious& u) { - // we are building new frame, so "current" is actually the previous frame. - auto previous = ::script::EngineScope::currentEngineAs(); + JscEngineScope::UnlockPrevious& u, JscEngine* previous) { if (previous && previous != currentEngine) { u.previousEngine_ = previous; previous->virtualMachineLock_->unlock(); diff --git a/backend/JavaScriptCore/JscScope.h b/backend/JavaScriptCore/JscScope.h index 354cf7c0..688c2cfc 100644 --- a/backend/JavaScriptCore/JscScope.h +++ b/backend/JavaScriptCore/JscScope.h @@ -29,8 +29,8 @@ class JscEngineScope { struct UnlockPrevious { JscEngine* previousEngine_; - explicit UnlockPrevious(JscEngine* currentEngine) : previousEngine_() { - UnlockPrevious_Ctor(currentEngine, *this); + explicit UnlockPrevious(JscEngine* currentEngine, JscEngine* previous) : previousEngine_() { + UnlockPrevious_Ctor(currentEngine, *this, previous); } ~UnlockPrevious() { UnlockPrevious_Dtor(*this); } @@ -40,12 +40,12 @@ class JscEngineScope { std::lock_guard lockGuard_; public: - explicit JscEngineScope(JscEngine&); + explicit JscEngineScope(JscEngine&, JscEngine*); ~JscEngineScope(); private: - static void UnlockPrevious_Ctor(JscEngine* engine, UnlockPrevious&); + static void UnlockPrevious_Ctor(JscEngine* engine, UnlockPrevious&, JscEngine* previous); static void UnlockPrevious_Dtor(UnlockPrevious&); }; diff --git a/backend/Lua/LuaScope.cc b/backend/Lua/LuaScope.cc index b28f7b23..df2d5e6b 100644 --- a/backend/Lua/LuaScope.cc +++ b/backend/Lua/LuaScope.cc @@ -46,8 +46,8 @@ StackFrameScopeImpl::~StackFrameScopeImpl() { } } -EngineScopeImpl::EngineScopeImpl(LuaEngine &engine) : lockGuard_(engine.lock_), stack_(engine) { - auto prevEngine = EngineScope::currentEngineAs(); +EngineScopeImpl::EngineScopeImpl(LuaEngine &engine, LuaEngine *prevEngine) + : lockGuard_(engine.lock_), stack_(engine) { if (prevEngine && prevEngine != stack_.engine_) { prevEngine_ = prevEngine; prevEngine->lock_.unlock(); diff --git a/backend/Lua/trait/TraitScope.h b/backend/Lua/trait/TraitScope.h index 3e57c811..104dd27c 100644 --- a/backend/Lua/trait/TraitScope.h +++ b/backend/Lua/trait/TraitScope.h @@ -51,7 +51,7 @@ class EngineScopeImpl { StackFrameScopeImpl stack_; public: - explicit EngineScopeImpl(LuaEngine &); + explicit EngineScopeImpl(LuaEngine &, LuaEngine *); ~EngineScopeImpl(); }; diff --git a/backend/QuickJs/QjsScope.cc b/backend/QuickJs/QjsScope.cc index 4ec45ff9..dc323300 100644 --- a/backend/QuickJs/QjsScope.cc +++ b/backend/QuickJs/QjsScope.cc @@ -20,8 +20,8 @@ namespace script::qjs_backend { -EngineScopeImpl::EngineScopeImpl(QjsEngine ¤t) - : previous_(EngineScope::currentEngineAs()), current_(¤t) { +EngineScopeImpl::EngineScopeImpl(QjsEngine ¤t, QjsEngine *prev) + : previous_(prev), current_(¤t) { if (previous_) { previous_->runtimeLock_.unlock(); } diff --git a/backend/QuickJs/trait/TraitScope.h b/backend/QuickJs/trait/TraitScope.h index 83caeabc..86c213b2 100644 --- a/backend/QuickJs/trait/TraitScope.h +++ b/backend/QuickJs/trait/TraitScope.h @@ -28,7 +28,7 @@ class EngineScopeImpl { QjsEngine *current_; public: - explicit EngineScopeImpl(QjsEngine &); + explicit EngineScopeImpl(QjsEngine &, QjsEngine *); ~EngineScopeImpl(); }; diff --git a/backend/Template/trait/TraitScope.h b/backend/Template/trait/TraitScope.h index 41c3809f..24d1f50f 100644 --- a/backend/Template/trait/TraitScope.h +++ b/backend/Template/trait/TraitScope.h @@ -25,7 +25,7 @@ namespace template_backend { class EngineScopeImpl { public: - explicit EngineScopeImpl(TemplateEngine &) { + explicit EngineScopeImpl(TemplateEngine &, TemplateEngine *) { // enter engine; } diff --git a/backend/V8/V8Scope.cc b/backend/V8/V8Scope.cc index 06cb4028..77825b01 100644 --- a/backend/V8/V8Scope.cc +++ b/backend/V8/V8Scope.cc @@ -22,7 +22,7 @@ namespace script::v8_backend { -V8EngineScope::V8EngineScope(V8Engine& engine) +V8EngineScope::V8EngineScope(V8Engine& engine, V8Engine*) : locker_(engine.isolate_), isolateScope_(engine.isolate_), handleScope_(engine.isolate_), diff --git a/backend/V8/V8Scope.h b/backend/V8/V8Scope.h index 3b5bac37..7a19129c 100644 --- a/backend/V8/V8Scope.h +++ b/backend/V8/V8Scope.h @@ -30,7 +30,7 @@ class V8EngineScope { v8::Context::Scope contextScope_; public: - explicit V8EngineScope(V8Engine& engine); + explicit V8EngineScope(V8Engine& engine, V8Engine* previous); ~V8EngineScope() = default; }; diff --git a/backend/WebAssembly/WasmScope.cc b/backend/WebAssembly/WasmScope.cc index fd5e3564..8ad9c79e 100644 --- a/backend/WebAssembly/WasmScope.cc +++ b/backend/WebAssembly/WasmScope.cc @@ -20,7 +20,7 @@ namespace script::wasm_backend { -WasmEngineScope::WasmEngineScope(WasmEngine &) : stackTop_(-1) { +WasmEngineScope::WasmEngineScope(WasmEngine &, WasmEngine *) : stackTop_(-1) { #ifdef __EMSCRIPTEN_PTHREADS__ if (wasm_backend::WasmEngine::engineThreadId_ != std::this_thread::get_id()) { std::ostringstream oss; diff --git a/backend/WebAssembly/trait/TraitScope.h b/backend/WebAssembly/trait/TraitScope.h index 2cc0ec49..c1166036 100644 --- a/backend/WebAssembly/trait/TraitScope.h +++ b/backend/WebAssembly/trait/TraitScope.h @@ -27,7 +27,7 @@ class WasmEngineScope { int stackTop_; public: - explicit WasmEngineScope(WasmEngine &); + explicit WasmEngineScope(WasmEngine &, WasmEngine *); ~WasmEngineScope() { // restore stack top diff --git a/src/Scope.cc b/src/Scope.cc index f2eca6da..160476f6 100644 --- a/src/Scope.cc +++ b/src/Scope.cc @@ -41,15 +41,14 @@ EngineScope::EngineScope(EngineImpl* engine) static inline EngineScope*& currentScope() { return internal::getThreadLocal(current_); } EngineScope::EngineScope(EngineScope::InternalEnterEngine, EngineImpl* engine, bool needEnter) - : needEnter_(needEnter && engine != nullptr && engine != currentEngine()), - engineScopeImpl_(), - engine_(engine), - prev_(currentScope()) { + : needEnter_(false), engineScopeImpl_(), engine_(engine), prev_(currentScope()) { + auto currentEngine = prev_ != nullptr ? prev_->engine_ : nullptr; + needEnter_ = needEnter && engine != nullptr && engine != currentEngine; if (needEnter_) { if (engine->isDestroying()) { throw std::logic_error("enter EngineScope with a destroying ScriptEngine"); } - engineScopeImpl_.emplace(*engine); + engineScopeImpl_.emplace(*engine, currentEngine); } currentScope() = this; @@ -86,6 +85,7 @@ ScriptEngine& EngineScope::currentEngineChecked() { ExitEngineScope::ExitEngineScope() : exitEngineScopeImpl_(EngineScope::currentEngineAs()), + // enters to a null engine, so EngineScope::currentEngine() == nullptr nullEngineScope_(EngineScope::InternalEnterEngine{}, nullptr) {} ExitEngineScope::ExitEngineHolder::ExitEngineHolder(ExitEngineScope::EngineImpl* engine) diff --git a/src/Scope.h b/src/Scope.h index acfc7f31..79f71aa4 100644 --- a/src/Scope.h +++ b/src/Scope.h @@ -48,7 +48,7 @@ class EngineScope final { * * class EngineScopeImpl { * public: - * EngineScopeImpl(EngineImpl&) { + * EngineScopeImpl(EngineImpl& engine, EngineImpl* prev) { * // enter engine; * } * From a03418c65fce5c219ae48aa97b9c47ba108e362c Mon Sep 17 00:00:00 2001 From: landerlyoung Date: Sat, 11 Dec 2021 18:30:55 +0800 Subject: [PATCH 3/4] enable UnitTest release build on ubuntu env --- .github/workflows/unit_tests.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/unit_tests.yml b/.github/workflows/unit_tests.yml index 644e9a57..8174410d 100644 --- a/.github/workflows/unit_tests.yml +++ b/.github/workflows/unit_tests.yml @@ -109,7 +109,7 @@ jobs: backends: [ V8, JavaScriptCore, QuickJs, Lua, Empty ] build_type: - Debug - #- Release + - Release steps: - uses: actions/checkout@v2 - uses: actions/cache@v2 @@ -150,7 +150,7 @@ jobs: backends: [ V8, JavaScriptCore, QuickJs, Lua, Empty ] build_type: - Debug - #- Release + - Release steps: - uses: actions/checkout@v2 - uses: actions/cache@v2 From ffe9e202b9f7f6d0fa8b171751604870f18f99e6 Mon Sep 17 00:00:00 2001 From: landerlyoung Date: Sat, 11 Dec 2021 18:47:57 +0800 Subject: [PATCH 4/4] explicitly mark Arbitrary data to be max aligned --- src/utils/MessageQueue.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/utils/MessageQueue.h b/src/utils/MessageQueue.h index 5569a1a9..7f6adc69 100644 --- a/src/utils/MessageQueue.h +++ b/src/utils/MessageQueue.h @@ -31,7 +31,7 @@ namespace script::utils { -struct ArbitraryData { +struct alignas(std::max_align_t) ArbitraryData { /** arbitrary data */ int64_t data0 = 0; int64_t data1 = 0; @@ -163,8 +163,7 @@ class InplaceMessage : public Message { throw std::runtime_error("inplaceObject can only be called once."); } - auto buffer = static_cast(static_cast(this)); - auto ptr = new (buffer) T(std::forward(args)...); + auto ptr = new (alignedStorage()) T(std::forward(args)...); cleanupProc = [](Message& self) { reinterpret_cast(self).getObject().~T(); }; @@ -173,12 +172,14 @@ class InplaceMessage : public Message { template T& getObject() { - return *reinterpret_cast(static_cast(this)); + return *static_cast(alignedStorage()); } private: InplaceMessage() = default; + void* alignedStorage() { return static_cast(static_cast(this)); } + friend class MessageQueue; friend class MemoryPool;