From b00e22b6b703be86d9be710f530107eae9b7401a Mon Sep 17 00:00:00 2001 From: Bart Janssens Date: Sun, 9 Jun 2024 17:24:49 +0200 Subject: [PATCH] Remove type map (#161) * Remove type map * Use hashmap for type mapping only on Windows See comments in jlcxx.cpp for details --- include/jlcxx/jlcxx_config.hpp | 5 +- include/jlcxx/module.hpp | 4 + include/jlcxx/smart_pointers.hpp | 31 ++++++-- include/jlcxx/stl.hpp | 2 +- include/jlcxx/type_conversion.hpp | 118 ++++++++++-------------------- src/jlcxx.cpp | 50 ++++++++----- 6 files changed, 102 insertions(+), 108 deletions(-) diff --git a/include/jlcxx/jlcxx_config.hpp b/include/jlcxx/jlcxx_config.hpp index 40d2a34..70ae063 100644 --- a/include/jlcxx/jlcxx_config.hpp +++ b/include/jlcxx/jlcxx_config.hpp @@ -2,6 +2,7 @@ #define JLCXX_CONFIG_HPP #ifdef _WIN32 + #define JLCXX_USE_TYPE_MAP #ifdef JLCXX_EXPORTS #define JLCXX_API __declspec(dllexport) #else @@ -14,8 +15,8 @@ #endif #define JLCXX_VERSION_MAJOR 0 -#define JLCXX_VERSION_MINOR 12 -#define JLCXX_VERSION_PATCH 5 +#define JLCXX_VERSION_MINOR 13 +#define JLCXX_VERSION_PATCH 0 // From https://stackoverflow.com/questions/5459868/concatenate-int-to-string-using-c-preprocessor #define __JLCXX_STR_HELPER(x) #x diff --git a/include/jlcxx/module.hpp b/include/jlcxx/module.hpp index 3a35f78..6385631 100644 --- a/include/jlcxx/module.hpp +++ b/include/jlcxx/module.hpp @@ -1235,6 +1235,10 @@ class TypeWrapper using TypeWrapper1 = TypeWrapper>>; +#ifdef JLCXX_USE_TYPE_MAP +JLCXX_API std::shared_ptr& jlcxx_smartpointer_type(std::type_index idx); +#endif + template using combine_types = typename CombineTypes::type; template diff --git a/include/jlcxx/smart_pointers.hpp b/include/jlcxx/smart_pointers.hpp index f482272..2dbd68b 100644 --- a/include/jlcxx/smart_pointers.hpp +++ b/include/jlcxx/smart_pointers.hpp @@ -186,13 +186,34 @@ struct SmartPtrMethods, OtherPtrT> } -JLCXX_API void set_smartpointer_type(const type_hash_t& hash, TypeWrapper1* new_wrapper); -JLCXX_API TypeWrapper1* get_smartpointer_type(const type_hash_t& hash); +template +inline std::shared_ptr& stored_smartpointer_type() +{ +#ifdef JLCXX_USE_TYPE_MAP + return jlcxx_smartpointer_type(typeid(T)); +#else + static std::shared_ptr m_ptr; + return m_ptr; +#endif +} + +template +void set_smartpointer_type(TypeWrapper1* new_wrapper) +{ + assert(stored_smartpointer_type().get() == nullptr); + stored_smartpointer_type().reset(new_wrapper); +} + +template +TypeWrapper1* get_smartpointer_type() +{ + return stored_smartpointer_type().get(); +} template class T> TypeWrapper1 smart_ptr_wrapper(Module& module) { - static TypeWrapper1* stored_wrapper = get_smartpointer_type(type_hash>()); + static TypeWrapper1* stored_wrapper = get_smartpointer_type>(); if(stored_wrapper == nullptr) { std::cerr << "Smart pointer type has no wrapper" << std::endl; @@ -220,7 +241,7 @@ template class T> TypeWrapper1& add_smart_pointer(Module& mod, const std::string& name) { TypeWrapper1* tw = new TypeWrapper1(mod.add_type>>(name, julia_type("SmartPointer", get_cxxwrap_module()))); - smartptr::set_smartpointer_type(type_hash>(), tw); + smartptr::set_smartpointer_type>(tw); return *tw; } @@ -284,7 +305,7 @@ struct julia_type_factory> detail::apply_smart_ptr_type()(curmod); smartptr::detail::SmartPtrMethods::type>::apply(curmod); assert(has_julia_type()); - return JuliaTypeCache::julia_type(); + return stored_type().get_dt(); } }; diff --git a/include/jlcxx/stl.hpp b/include/jlcxx/stl.hpp index a406495..5db7911 100644 --- a/include/jlcxx/stl.hpp +++ b/include/jlcxx/stl.hpp @@ -351,7 +351,7 @@ struct julia_type_factory> Module& curmod = registry().current_module(); stl::apply_stl(curmod); assert(has_julia_type()); - return JuliaTypeCache::julia_type(); + return stored_type().get_dt(); } }; diff --git a/include/jlcxx/type_conversion.hpp b/include/jlcxx/type_conversion.hpp index d8f84c7..df0887d 100644 --- a/include/jlcxx/type_conversion.hpp +++ b/include/jlcxx/type_conversion.hpp @@ -322,7 +322,7 @@ struct static_type_mapping> template using static_julia_type = typename static_type_mapping::type; // Store a data type pointer, ensuring GC safety -struct CachedDatatype +struct JLCXX_API CachedDatatype { explicit CachedDatatype() : m_dt(nullptr) {} explicit CachedDatatype(jl_datatype_t* dt, bool protect = true) @@ -348,112 +348,64 @@ struct CachedDatatype jl_datatype_t* m_dt = nullptr; }; -// Work around the fact that references aren't part of the typeid result -using type_hash_t = std::pair; -} // namespace jlcxx +#ifdef JLCXX_USE_TYPE_MAP -namespace std { - -// Hash implementation from https://en.cppreference.com/w/cpp/utility/hash -template<> -struct hash -{ - std::size_t operator()(const jlcxx::type_hash_t& h) const noexcept - { - std::size_t h1 = std::hash{}(h.first); - std::size_t h2 = std::hash{}(h.second); - return h1 ^ (h2 << 1); - } -}; - -} - -namespace jlcxx -{ - -namespace detail -{ +JLCXX_API CachedDatatype& jlcxx_type(std::type_index idx); +JLCXX_API CachedDatatype& jlcxx_reftype(std::type_index idx); +JLCXX_API CachedDatatype& jlcxx_constreftype(std::type_index idx); template -struct TypeHash +struct HashedCache { - static inline type_hash_t value() + static inline CachedDatatype& value() { - return std::make_pair(std::type_index(typeid(T)), std::size_t(0)); + return jlcxx_type(typeid(T)); } }; template -struct TypeHash +struct HashedCache { - static inline type_hash_t value() + static inline CachedDatatype& value() { - return std::make_pair(std::type_index(typeid(T)), std::size_t(1)); + return jlcxx_reftype(typeid(T)); } }; template -struct TypeHash +struct HashedCache { - static inline type_hash_t value() + static inline CachedDatatype& value() { - return std::make_pair(std::type_index(typeid(T)), std::size_t(2)); + return jlcxx_constreftype(typeid(T)); } }; -} +#endif -template -inline type_hash_t type_hash() +template +CachedDatatype& stored_type() { - return detail::TypeHash::value(); +#ifdef JLCXX_USE_TYPE_MAP + return HashedCache::value(); +#else + static CachedDatatype m_dt; + return m_dt; +#endif } -JLCXX_API std::unordered_map& jlcxx_type_map(); - -/// Store the Julia datatype linked to SourceT -template -class JuliaTypeCache -{ -public: - - static inline jl_datatype_t* julia_type() - { - const auto result = jlcxx_type_map().find(type_hash()); - if(result == jlcxx_type_map().end()) - { - throw std::runtime_error("Type " + std::string(typeid(SourceT).name()) + " has no Julia wrapper"); - } - return result->second.get_dt(); - } - - static inline void set_julia_type(jl_datatype_t* dt, bool protect = true) - { - type_hash_t new_hash = type_hash(); - const auto [inserted_it, insert_success] = jlcxx_type_map().insert(std::make_pair(new_hash, CachedDatatype(dt, protect))); - if(!insert_success) - { - type_hash_t old_hash = inserted_it->first; - std::cout << "Warning: Type " << new_hash.first.name() << " already had a mapped type set as " - << julia_type_name(inserted_it->second.get_dt()) << " and const-ref indicator " << old_hash.second << " and C++ type name " << old_hash.first.name() - << ". Hash comparison: old(" << old_hash.first.hash_code() << "," << old_hash.second << ") == new(" << old_hash.first.hash_code() << "," << old_hash.second << ") == " - << std::boolalpha << (old_hash == new_hash) << std::endl; - return; - } - } - - static inline bool has_julia_type() - { - const std::size_t nb_hits = jlcxx_type_map().count(type_hash()); - return nb_hits != 0; - } -}; - template void set_julia_type(jl_datatype_t* dt, bool protect = true) { - JuliaTypeCache::type>::set_julia_type(dt, protect); + using nonconst_t = typename std::remove_const::type; + CachedDatatype& cache = stored_type(); + if(cache.get_dt() != nullptr) + { + std::cout << "Warning: Type " << typeid(T).name() << " already had a mapped type set as " << julia_type_name(cache.get_dt()) << std::endl; + return; + } + cache.set_dt(dt, protect); } /// Store the Julia datatype linked to SourceT @@ -473,7 +425,11 @@ template inline jl_datatype_t* julia_type() { using nonconst_t = typename std::remove_const::type; - static jl_datatype_t* dt = JuliaTypeCache::julia_type(); + jl_datatype_t* dt = stored_type().get_dt(); + if(dt == nullptr) + { + throw std::runtime_error("Type " + std::string(typeid(nonconst_t).name()) + " has no Julia wrapper"); + } return dt; } @@ -482,7 +438,7 @@ template bool has_julia_type() { using nonconst_t = typename std::remove_const::type; - return JuliaTypeCache::has_julia_type(); + return stored_type().get_dt() != nullptr; } /// Create the julia type associated with the given C++ type diff --git a/src/jlcxx.cpp b/src/jlcxx.cpp index 6316e22..f52ee86 100644 --- a/src/jlcxx.cpp +++ b/src/jlcxx.cpp @@ -341,38 +341,50 @@ namespace detail }; } -JLCXX_API std::unordered_map& jlcxx_type_map() -{ - static std::unordered_map m_map; - return m_map; -} +#ifdef JLCXX_USE_TYPE_MAP -namespace smartptr -{ +// On windows, we can't store a mapping from the C++ type to the Julia type +// in a static variable declared in a template function, since each DLL +// will have its onw copy of that variable, making it impossible to share +// type definitions between the CxxWrap base library and other libraries. +// The workaround is to store the types in a map, but this is more fragile +// because the guarantees on std::type_index are not very strong either +// in the context of sharing information between shared libraries. This is +// why we fall back to this approach only on Windows. Using type names is +// also not a solution because types in anonymous namespaces will clash. +// Refs: +// https://stackoverflow.com/questions/398069/static-member-variable-in-template-with-multiple-dlls +// https://developercommunity.visualstudio.com/t/template-static-members-and-multiple-definitions-a/1202888 +// https://github.com/pybind/pybind11/pull/4319 +// https://bugs.llvm.org/show_bug.cgi?id=33542 +// https://github.com/pybind/pybind11/issues/3289 -std::map>& jlcxx_smartpointer_types() +JLCXX_API CachedDatatype& jlcxx_type(std::type_index idx) { - static std::map> m_map; - return m_map; + static std::unordered_map m_map; + return m_map.insert(std::make_pair(idx,CachedDatatype())).first->second; } -JLCXX_API void set_smartpointer_type(const type_hash_t& hash, TypeWrapper1* new_wrapper) +JLCXX_API CachedDatatype& jlcxx_reftype(std::type_index idx) { - jlcxx_smartpointer_types()[hash] = std::shared_ptr(new_wrapper); + static std::unordered_map m_map; + return m_map.insert(std::make_pair(idx,CachedDatatype())).first->second; } -JLCXX_API TypeWrapper1* get_smartpointer_type(const type_hash_t& hash) +JLCXX_API CachedDatatype& jlcxx_constreftype(std::type_index idx) { - auto result = jlcxx_smartpointer_types().find(hash); - if(result == jlcxx_smartpointer_types().end()) - { - return nullptr; - } - return result->second.get(); + static std::unordered_map m_map; + return m_map.insert(std::make_pair(idx,CachedDatatype())).first->second; } +JLCXX_API std::shared_ptr& jlcxx_smartpointer_type(std::type_index idx) +{ + static std::unordered_map> m_map; + return m_map.insert(std::make_pair(idx, nullptr)).first->second; } +#endif + JLCXX_API void register_core_types() { if(jl_base_module == nullptr)