tqchen commented on code in PR #658:
URL: https://github.com/apache/tvm-ffi/pull/658#discussion_r3537195670
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_session.cc:
##########
@@ -203,9 +204,53 @@ ORCJITExecutionSession::ORCJITExecutionSession(const
std::string& orc_rt_path,
data_ = std::move(obj);
}
+//-------------------------------------
+// Default ORC runtime path registry
+//-------------------------------------
+
+namespace {
+// Small settable registry for the process-wide default ORC runtime path, using
+// the leaked-singleton idiom (never torn down during interpreter
finalization).
+struct DefaultOrcRuntimePath {
+ std::mutex mutex;
+ String path;
+
+ static DefaultOrcRuntimePath* Global() {
+ static auto* inst = new DefaultOrcRuntimePath();
Review Comment:
this is not needed, maybe we can simply have python register the
tvm_ffi_orcjit.DefaultOrcRuntimePath function
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_session.h:
##########
@@ -58,13 +80,60 @@ class ORCJITExecutionSessionObj : public Object {
explicit ORCJITExecutionSessionObj(const std::string& orc_rt_path = "",
int64_t slab_size_bytes = 0);
+ /*!
+ * \brief Get the process-wide shared execution session.
+ *
+ * A leaked, never-destroyed singleton so multiple callers in one process
+ * share one LLVM ExecutionSession — hence process symbols, the slab arena,
+ * and cross-library linking. Never torn down (interpreter finalization could
+ * otherwise call back into the host language during teardown). Created on
+ * first call using \ref GetDefaultOrcRuntimePath.
+ *
+ * \return The shared execution session.
+ */
+ static ORCJITExecutionSession Default();
Review Comment:
GlobalDefault
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_session.cc:
##########
@@ -203,9 +204,53 @@ ORCJITExecutionSession::ORCJITExecutionSession(const
std::string& orc_rt_path,
data_ = std::move(obj);
}
+//-------------------------------------
+// Default ORC runtime path registry
+//-------------------------------------
+
+namespace {
+// Small settable registry for the process-wide default ORC runtime path, using
+// the leaked-singleton idiom (never torn down during interpreter
finalization).
+struct DefaultOrcRuntimePath {
+ std::mutex mutex;
+ String path;
+
+ static DefaultOrcRuntimePath* Global() {
+ static auto* inst = new DefaultOrcRuntimePath();
+ return inst;
+ }
+};
+} // namespace
+
+void SetDefaultOrcRuntimePath(const String& path) {
+ auto* reg = DefaultOrcRuntimePath::Global();
+ std::lock_guard<std::mutex> lock(reg->mutex);
+ reg->path = path;
+}
+
+String GetDefaultOrcRuntimePath() {
+ auto* reg = DefaultOrcRuntimePath::Global();
+ std::lock_guard<std::mutex> lock(reg->mutex);
+ return reg->path;
+}
+
+ORCJITExecutionSession ORCJITExecutionSessionObj::Default() {
+ // Leaked, never-destroyed: the owned LLJIT and slab arena must not be torn
+ // down during interpreter finalization, where teardown could call back into
+ // the host language. Resolve the ORC runtime path from the settable registry
+ // (empty if unset — platform default / disabled).
+ static ORCJITExecutionSession* inst =
+ new ORCJITExecutionSession(GetDefaultOrcRuntimePath().operator
std::string(), 0);
+ return *inst;
+}
+
ORCJITDynamicLibrary ORCJITExecutionSessionObj::CreateDynamicLibrary(const
String& name) {
TVM_FFI_CHECK(jit_ != nullptr, InternalError) << "ExecutionSession not
initialized";
+ // Compound topology op — serialize against concurrent create / add / lookup
/
+ // teardown on this shared session (lock order: session lock first).
+ std::lock_guard<std::recursive_mutex> lock(session_mutex_);
Review Comment:
do we need recursive muted here? seems normal one is fine
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_dylib.cc:
##########
@@ -225,15 +398,22 @@ static void RegisterOrcJITFunctions() {
refl::ObjectDef<ORCJITExecutionSessionObj>();
refl::GlobalDef()
- .def("orcjit.ExecutionSession",
+ .def("ffi_orcjit.ExecutionSession",
[](const std::string& orc_rt_path, int64_t slab_size_bytes) {
return ORCJITExecutionSession(orc_rt_path, slab_size_bytes);
})
- .def("orcjit.ExecutionSessionCreateDynamicLibrary",
+ .def("ffi_orcjit.DefaultSession", []() { return
ORCJITExecutionSessionObj::Default(); })
Review Comment:
consider write tvm_ffi_orcjit as full path name
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_session.h:
##########
@@ -58,13 +80,60 @@ class ORCJITExecutionSessionObj : public Object {
explicit ORCJITExecutionSessionObj(const std::string& orc_rt_path = "",
int64_t slab_size_bytes = 0);
+ /*!
+ * \brief Get the process-wide shared execution session.
+ *
+ * A leaked, never-destroyed singleton so multiple callers in one process
+ * share one LLVM ExecutionSession — hence process symbols, the slab arena,
+ * and cross-library linking. Never torn down (interpreter finalization could
+ * otherwise call back into the host language during teardown). Created on
+ * first call using \ref GetDefaultOrcRuntimePath.
+ *
+ * \return The shared execution session.
+ */
+ static ORCJITExecutionSession Default();
+
/*!
* \brief Create a new DynamicLibrary (JITDylib) in this session
* \param name Optional name for the library (for debugging)
* \return The created dynamic library instance
*/
ORCJITDynamicLibrary CreateDynamicLibrary(const String& name);
+ /*!
+ * \brief Load a set of objects into one fresh dynamic library and return the
+ * resulting module, fully wired.
+ *
+ * End-to-end high-level entry: creates a JITDylib, adds every object (a
+ * \c String path or in-memory \c Bytes image), injects context symbols
+ * eagerly, and — if the objects embed a library binary — reconstructs the
+ * import tree so the result behaves like a normally-loaded tvm-ffi module.
+ * The whole sequence runs under one recursive session lock, and the dylib is
+ * never exposed in a partially-loaded state, so finalization happens exactly
+ * once and cannot be repeated or interleaved.
+ *
+ * \param objects Array whose elements are each a \c String path or \c Bytes
+ * object-file image.
+ * \param name Optional JITDylib name (auto-generated when empty).
+ * \return The root module, with imports and library context fully wired (the
+ * dylib itself when there is no embedded library binary).
+ */
+ Module LoadModule(const Array<Any>& objects, const String& name);
+
+ /*!
+ * \brief The session-wide recursive lock serializing compound JITDylib ops.
+ *
+ * Compound topology operations (create / add object / set link order /
+ * symbol lookup+init / teardown) are individually locked by LLVM but not
+ * atomic with respect to one another; a shared session driven by multiple
+ * threads (the FFI call path releases the GIL) must serialize them.
Recursive
+ * because they nest on one thread (create → JIT'd constructor → function
+ * lookup). The already-resolved-call hot path never takes it.
+ *
+ * \return Reference to the recursive session mutex.
+ */
+ std::recursive_mutex& mutex() { return session_mutex_; }
Review Comment:
likely no need to expose
##########
addons/tvm_ffi_orcjit/src/ffi/orcjit_dylib.cc:
##########
@@ -198,6 +366,11 @@ Optional<Function>
ORCJITDynamicLibraryObj::GetFunction(const String& name) {
// TVM-FFI exports have __tvm_ffi_ prefix
std::string symbol_name = symbol::tvm_ffi_symbol_prefix + std::string(name);
+ // Compound lookup+init: serialize against concurrent session operations
+ // (lock order: session lock → refresh mutex → LLVM lookup). The returned
+ // Function, once resolved, is invoked lock-free on the hot path.
Review Comment:
i think we can further simplify if we assume ORCJITDynamicLibrary is unit
operation, we explicitly do refresh in LoadModule, then GetFunction do not need
to do that, avoid exposing functions like SetLinkOrder AddObjectBytes, leave
those as internal
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
