From df454a0c4e0c78baf509d3ab28db7c2d78b6121f Mon Sep 17 00:00:00 2001 From: Petr Ogarok Date: Thu, 22 Feb 2024 11:59:48 -0500 Subject: [PATCH 1/5] Switch lack of memoization logging level to DEBUG instead of WARN Signed-off-by: Tim Paine --- csp/impl/mem_cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/csp/impl/mem_cache.py b/csp/impl/mem_cache.py index 064fed5a..f4886465 100644 --- a/csp/impl/mem_cache.py +++ b/csp/impl/mem_cache.py @@ -206,7 +206,7 @@ def __call__(*args, **kwargs): raise if not warned_flag.value: logging_context = function_name if function_name else str(func) - logging.warning(f"Not memoizing output of {str(logging_context)}: {str(e)}") + logging.debug(f"Not memoizing output of {str(logging_context)}: {str(e)}") warned_flag.value = True cur_item = func(*args, **kwargs) else: From a25da27c9e855e6d4abccd9ad28bedd0103ebf59 Mon Sep 17 00:00:00 2001 From: Rob Ambalu Date: Thu, 22 Feb 2024 12:04:21 -0500 Subject: [PATCH 2/5] struct bugfix on disappearing dynamic types, fixes #74 Signed-off-by: Tim Paine --- cpp/csp/engine/Struct.cpp | 59 ++++++++++++++++++----------------- cpp/csp/engine/Struct.h | 42 ++++++++++++------------- cpp/csp/python/PyStruct.cpp | 20 ++++++++++-- csp/tests/impl/test_struct.py | 22 ++++++++++--- 4 files changed, 87 insertions(+), 56 deletions(-) diff --git a/cpp/csp/engine/Struct.cpp b/cpp/csp/engine/Struct.cpp index 57166fd8..7282984b 100644 --- a/cpp/csp/engine/Struct.cpp +++ b/cpp/csp/engine/Struct.cpp @@ -5,7 +5,7 @@ namespace csp { -StructField::StructField( CspTypePtr type, const std::string & fieldname, +StructField::StructField( CspTypePtr type, const std::string & fieldname, size_t size, size_t alignment ) : m_fieldname( fieldname ), m_offset( 0 ), @@ -18,7 +18,7 @@ StructField::StructField( CspTypePtr type, const std::string & fieldname, { } -/* StructMeta +/* StructMeta A note on member layout. Meta will order objects in the following order: - non-native fields ( ie PyObjectPtr for the python dialect ) @@ -26,7 +26,7 @@ A note on member layout. Meta will order objects in the following order: - set/unset bitmask bytes ( 1 byte per 8 fields ) Derived structs will simply append to the layout of the base struct, properly padding between classes to align -its fields properly. +its fields properly. This layout is imposed on Struct instances. Since Struct needs refcount and meta * fields, for convenience they are stored *before* a Struct's "this" pointer as hidden data. This way struct ptrs can be passed into StructMeta without and adjustments required for the hidden fields @@ -47,7 +47,7 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields, //decided to place them at the start cause they are most likely size of ptr or greater m_fieldnames.reserve( m_fields.size() ); - for( size_t i = 0; i < m_fields.size(); i++ ) + for( size_t i = 0; i < m_fields.size(); i++ ) m_fieldnames.emplace_back( m_fields[i] -> fieldname() ); std::sort( m_fields.begin(), m_fields.end(), []( auto && a, auto && b ) @@ -93,7 +93,7 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields, m_isFullyNative = m_isPartialNative && ( m_base ? m_base -> isNative() : true ); //Setup masking bits for our fields - //NOTE we can be more efficient by sticking masks into any potential alignment gaps, dont want to spend time on it + //NOTE we can be more efficient by sticking masks into any potential alignment gaps, dont want to spend time on it //at this point m_maskSize = !m_fields.empty() ? 1 + ( ( m_fields.size() - 1 ) / 8 ) : 0; m_size = offset + m_maskSize; @@ -116,7 +116,7 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields, { m_fields.insert( m_fields.begin(), m_base -> m_fields.begin(), m_base -> m_fields.end() ); m_fieldnames.insert( m_fieldnames.begin(), m_base -> m_fieldnames.begin(), m_base -> m_fieldnames.end() ); - + m_firstPartialField = m_base -> m_fields.size(); m_firstNativePartialField += m_base -> m_fields.size(); m_fieldMap = m_base -> m_fieldMap; @@ -138,9 +138,9 @@ StructMeta::~StructMeta() Struct * StructMeta::createRaw() const { Struct * s; - //This weird looking new call actually is calling Struct::opereatore new( size_t, StructMeta * ) + //This weird looking new call actually is calling Struct::opereatore new( size_t, std::shared_ptr ) //its not actually doing an in-place on StructMeta this - s = new ( this ) Struct( this ); + s = new ( shared_from_this() ) Struct( shared_from_this() ); initialize( s ); @@ -212,7 +212,7 @@ void StructMeta::initialize( Struct * s ) const } memset( reinterpret_cast(s) + m_nativeStart, 0, partialNativeSize() ); - + if( !m_isPartialNative ) { for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx ) @@ -221,7 +221,7 @@ void StructMeta::initialize( Struct * s ) const static_cast( field ) -> initialize( s ); } } - + if( m_base ) m_base -> initialize( s ); } @@ -231,13 +231,13 @@ void StructMeta::copyFrom( const Struct * src, Struct * dest ) if( unlikely( src == dest ) ) return; - if( dest -> meta() != src -> meta() && + if( dest -> meta() != src -> meta() && !StructMeta::isDerivedType( src -> meta(), dest -> meta() ) ) - CSP_THROW( TypeError, "Attempting to copy from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name() + CSP_THROW( TypeError, "Attempting to copy from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name() << "'. copy_from may only be used to copy from same type or derived types" ); dest -> meta() -> copyFromImpl( src, dest ); -} +} void StructMeta::copyFromImpl( const Struct * src, Struct * dest ) const { @@ -264,7 +264,7 @@ void StructMeta::copyFromImpl( const Struct * src, Struct * dest ) const //note that partialNative will include the mask bytes - this sets the native part and the mask memcpy( reinterpret_cast(dest) + m_nativeStart, reinterpret_cast(src) + m_nativeStart, partialNativeSize() ); - + if( m_base ) m_base -> copyFromImpl( src, dest ); } @@ -275,13 +275,13 @@ void StructMeta::updateFrom( const Struct * src, Struct * dest ) if( unlikely( src == dest ) ) return; - if( dest -> meta() != src -> meta() && + if( dest -> meta() != src -> meta() && !StructMeta::isDerivedType( src -> meta(), dest -> meta() ) ) - CSP_THROW( TypeError, "Attempting to update from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name() + CSP_THROW( TypeError, "Attempting to update from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name() << "'. update_from may only be used to update from same type or derived types" ); dest -> meta() -> updateFromImpl( src, dest ); -} +} void StructMeta::updateFromImpl( const Struct * src, Struct * dest ) const { @@ -312,7 +312,7 @@ bool StructMeta::isEqual( const Struct * x, const Struct * y ) const //Note the curent use of memcpy for native types. This can cause issues on double comparisons //esp if expecting NaN == NaN to be false, and when comparing -0.0 to +0.0.. may want to revisit - //We we do we may as well remove the basepadding copy + //We we do we may as well remove the basepadding copy if( isNative() ) return memcmp( x, y, size() ) == 0; @@ -325,7 +325,7 @@ bool StructMeta::isEqual( const Struct * x, const Struct * y ) const for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx ) { auto * field = m_fields[ idx ].get(); - + if( field -> isSet( x ) != field -> isSet( y ) ) return false; @@ -370,19 +370,19 @@ size_t StructMeta::hash( const Struct * x ) const "Exceeded max recursion depth of " << MAX_RECURSION_DEPTH << " in " << name() << "::hash(), cannot hash cyclic data structure" ); hash ^= csp::hash::hash_bytes( x + m_nativeStart, partialNativeSize() ); - + if( !m_isPartialNative ) { for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx ) { auto * field = m_fields[ idx ].get(); - + //we dont incorporate unset fields, bitmask will cover them if( field -> isSet( x ) ) hash ^= static_cast( field ) -> hash( x ); } } - + if( m_base ) hash ^= std::hash()( (uint64_t ) m_base.get() ) ^ m_base -> hash( x ); @@ -398,18 +398,18 @@ void StructMeta::clear( Struct * s ) const } memset( reinterpret_cast(s) + m_nativeStart, 0, partialNativeSize() ); - + if( !m_isPartialNative ) { for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx ) { auto * field = m_fields[ idx ].get(); - + if( field -> isSet( s ) ) static_cast( field ) -> clearValue( s ); } } - + if( m_base ) m_base -> clear( s ); } @@ -450,19 +450,22 @@ void StructMeta::destroy( Struct * s ) const static_cast( field ) -> destroy( s ); } } - + if( m_base ) m_base -> destroy( s ); } -Struct::Struct( const StructMeta * meta ) +Struct::Struct( const std::shared_ptr & meta ) { + //Initialize meta shared_ptr + new( hidden() ) HiddenData(); + hidden() -> refcount = 1; hidden() -> meta = meta; hidden() -> dialectPtr = nullptr; } -void * Struct::operator new( std::size_t count, const StructMeta * meta ) +void * Struct::operator new( std::size_t count, const std::shared_ptr & meta ) { //allocate meta -> size() ( data members ) + room for hidden data refcount and meta ptr //Note that we currently assume sizeof( HiddenData ) is 16. We expect it to be a multiple of 8 diff --git a/cpp/csp/engine/Struct.h b/cpp/csp/engine/Struct.h index 40ab5d1a..7b3464b3 100644 --- a/cpp/csp/engine/Struct.h +++ b/cpp/csp/engine/Struct.h @@ -21,7 +21,7 @@ using StructPtr = TypedStructPtr; class StructField { public: - + virtual ~StructField() {} const std::string & fieldname() const { return m_fieldname; } @@ -31,7 +31,7 @@ class StructField size_t alignment() const { return m_alignment; } //alignment of the field size_t maskOffset() const { return m_maskOffset; } //offset to location of the mask byte fo this field from start of struct mem uint8_t maskBit() const { return m_maskBit; } //bit within mask byte associated with this field - uint8_t maskBitMask() const { return m_maskBitMask; } //same as maskBit but as a mask ( 1 << bit + uint8_t maskBitMask() const { return m_maskBitMask; } //same as maskBit but as a mask ( 1 << bit bool isNative() const { return m_type -> type() <= CspType::Type::MAX_NATIVE_TYPE; } @@ -41,8 +41,8 @@ class StructField CSP_ASSERT( bit < 8 ); m_maskOffset = off; - m_maskBit = bit; - m_maskBitMask = 1 << bit; + m_maskBit = bit; + m_maskBitMask = 1 << bit; } bool isSet( const Struct * s ) const @@ -72,7 +72,7 @@ class StructField protected: - StructField( CspTypePtr type, const std::string & fieldname, + StructField( CspTypePtr type, const std::string & fieldname, size_t size, size_t alignment ); void setIsSet( Struct * s ) const @@ -80,14 +80,14 @@ class StructField uint8_t * m = reinterpret_cast( s ) + m_maskOffset; (*m) |= m_maskBitMask; } - + const void * valuePtr( const Struct * s ) const - { + { return valuePtr( const_cast( s ) ); } void * valuePtr( Struct * s ) const - { + { return reinterpret_cast( s ) + m_offset; } @@ -96,7 +96,7 @@ class StructField uint8_t * m = reinterpret_cast( s ) + m_maskOffset; (*m) &= ~m_maskBitMask; } - + private: std::string m_fieldname; size_t m_offset; @@ -507,7 +507,7 @@ class TypedStructPtr void incref(); T * m_obj; - + template friend class TypedStructPtr; @@ -524,7 +524,7 @@ TypedStructPtr structptr_cast( const TypedStructPtr & r ) return out; } -class StructMeta +class StructMeta : public std::enable_shared_from_this { public: using Fields = std::vector; @@ -539,7 +539,7 @@ class StructMeta size_t partialSize() const { return m_partialSize; } bool isNative() const { return m_isFullyNative; } - + const Fields & fields() const { return m_fields; } const FieldNames & fieldNames() const { return m_fieldnames; } @@ -547,10 +547,10 @@ class StructMeta size_t maskSize() const { return m_maskSize; } const StructFieldPtr & field( const char * name ) const - { + { static StructFieldPtr s_empty; - auto it = m_fieldMap.find( name ); - return it == m_fieldMap.end() ? s_empty : it -> second; + auto it = m_fieldMap.find( name ); + return it == m_fieldMap.end() ? s_empty : it -> second; } const StructFieldPtr & field( const std::string & name ) const @@ -631,7 +631,7 @@ class Struct { public: - const StructMeta * meta() const { return hidden() -> meta; } + const StructMeta * meta() const { return hidden() -> meta.get(); } size_t refcount() const { return hidden() -> refcount; } bool operator==( const Struct & rhs ) const { return meta() -> isEqual( this, &rhs ); } @@ -673,10 +673,10 @@ class Struct void setDialectPtr( void * p ) { hidden() -> dialectPtr = p; } private: - static void * operator new( std::size_t count, const StructMeta * meta ); + static void * operator new( std::size_t count, const std::shared_ptr & meta ); static void operator delete( void * ptr ); - Struct( const StructMeta * meta ); + Struct( const std::shared_ptr & meta ); ~Struct() { meta() -> destroy( this ); @@ -703,7 +703,7 @@ class Struct struct HiddenData { size_t refcount; - const StructMeta * meta; + std::shared_ptr meta; void * dialectPtr; }; @@ -730,7 +730,7 @@ template inline void TypedStructPtr::decref() { m_obj -> decref(); -} +} template @@ -746,7 +746,7 @@ bool TypedStructPtr::operator==( const TypedStructPtr & rhs ) const class StructStructField final : public NonNativeStructField { public: - StructStructField( CspTypePtr cspType, const std::string &fieldname ) : + StructStructField( CspTypePtr cspType, const std::string &fieldname ) : NonNativeStructField( cspType, fieldname, sizeof( StructPtr ), alignof( StructPtr ) ) { CSP_ASSERT( cspType -> type() == CspType::Type::STRUCT ); diff --git a/cpp/csp/python/PyStruct.cpp b/cpp/csp/python/PyStruct.cpp index 00a81b4b..0128f096 100644 --- a/cpp/csp/python/PyStruct.cpp +++ b/cpp/csp/python/PyStruct.cpp @@ -167,8 +167,22 @@ static PyObject * PyStructMeta_new( PyTypeObject *subtype, PyObject *args, PyObj } } - //back reference to the struct type that will be accessible on the csp struct -> meta() - //DialectStructMeta will hold a borrowed reference to the type to avoid a circular dep + /*back reference to the struct type that will be accessible on the csp struct -> meta() + DialectStructMeta will hold a borrowed reference to the type to avoid a circular dep + + This is the layout of references between all these types + StructMeta (shared_ptr) <-------- strong ref + | | + DialectStructMeta ---> weak ref to PyStructMeta ( the PyType ) + /\ /\ + | | + | (strong ref ) | + Struct (shared_ptr) python ref to its PyType + /\ | + | | + | | + PyStruct -------------------------- + */ auto structMeta = std::make_shared( ( PyTypeObject * ) pymeta, name, fields, metabase ); //Setup fast attr dict lookup @@ -454,7 +468,7 @@ void format_double( const double val, std::string & tl_repr ) } void format_pyobject( const PyObjectPtr & pyptr, std::string & tl_repr ) { - auto repr = PyObjectPtr::own( PyObject_Repr( pyptr.get() ) ); + auto repr = PyObjectPtr::check( PyObject_Repr( pyptr.get() ) ); tl_repr += ( char * ) PyUnicode_DATA(repr.get() ); } diff --git a/csp/tests/impl/test_struct.py b/csp/tests/impl/test_struct.py index 4c67b3eb..5aedc14f 100644 --- a/csp/tests/impl/test_struct.py +++ b/csp/tests/impl/test_struct.py @@ -4,6 +4,7 @@ from datetime import date, datetime, time, timedelta import csp +from csp.impl.struct import defineStruct class MyEnum(csp.Enum): @@ -588,16 +589,16 @@ class S(csp.Struct): i: 2 f: 2.5 b: false - ls: - - 1 - - 2 + ls: + - 1 + - 2 - 3 lc: - value: [1,2,3] set_value: ["x","y","z"] - - value: + value: - 4 """ @@ -1064,6 +1065,19 @@ class StructB(csp.Struct): b.x = b self.assertEqual(repr(b), "StructB( x=( ... ) )") + def test_disappearing_dynamic_types(self): + """Was a BUG due to missing refcount: https://github.com/Point72/csp/issues/74""" + + class Outer(csp.Struct): + s: csp.Struct + + all = [] + for i in range(10000): + sType = defineStruct("foo", {"a": dict}) + all.append(Outer(s=sType(a={"foo": "bar"}))) + repr(all) + all = all[:100] + if __name__ == "__main__": unittest.main() From 01aeb749f212379b78de8844731d088035de059b Mon Sep 17 00:00:00 2001 From: Tim Paine Date: Thu, 22 Feb 2024 12:04:39 -0500 Subject: [PATCH 3/5] Autoformat python files Signed-off-by: Tim Paine --- csp/impl/wiring/edge.py | 1 - csp/tests/test_examples.py | 1 + csp/tests/test_stats.py | 6 +++--- examples/3_using_adapters/e_11_websocket_output.py | 4 +++- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/csp/impl/wiring/edge.py b/csp/impl/wiring/edge.py index ba4fa05d..0c187ab3 100644 --- a/csp/impl/wiring/edge.py +++ b/csp/impl/wiring/edge.py @@ -1,4 +1,3 @@ - class Edge: __slots__ = ["tstype", "nodedef", "output_idx", "basket_idx"] diff --git a/csp/tests/test_examples.py b/csp/tests/test_examples.py index b0d076c1..f0694c6d 100644 --- a/csp/tests/test_examples.py +++ b/csp/tests/test_examples.py @@ -24,6 +24,7 @@ def _get_modules_to_test(folder): if file.endswith(".py") ] + def _no_examples_folder_or_running_sdist_tests(): return os.environ.get("CSP_TEST_SKIP_EXAMPLES", None) is not None or not os.path.exists(EXAMPLES_ROOT) diff --git a/csp/tests/test_stats.py b/csp/tests/test_stats.py index 67b8f248..c1e1525c 100644 --- a/csp/tests/test_stats.py +++ b/csp/tests/test_stats.py @@ -2629,9 +2629,9 @@ def test_listbasket(self): st = datetime(2020, 1, 1) @csp.node - def takes_1darray( x: csp.ts[csp.typing.Numpy1DArray[float]] ) -> csp.ts[csp.typing.Numpy1DArray[float]]: + def takes_1darray(x: csp.ts[csp.typing.Numpy1DArray[float]]) -> csp.ts[csp.typing.Numpy1DArray[float]]: return x - + @csp.graph def graph(): x_np = csp.curve( @@ -2647,7 +2647,7 @@ def graph(): as_np = csp.stats.list_to_numpy([x_list1, x_list2]) as_list = csp.stats.numpy_to_list(x_np, 2) - csp.add_graph_output("as_np", takes_1darray( as_np ) ) + csp.add_graph_output("as_np", takes_1darray(as_np)) csp.add_graph_output("as_list0", as_list[0]) csp.add_graph_output("as_list1", as_list[1]) diff --git a/examples/3_using_adapters/e_11_websocket_output.py b/examples/3_using_adapters/e_11_websocket_output.py index de34aa42..7ef9b6d0 100755 --- a/examples/3_using_adapters/e_11_websocket_output.py +++ b/examples/3_using_adapters/e_11_websocket_output.py @@ -40,7 +40,9 @@ def main(port: int, num_keys: int): r = delayed_angle / math.pi s = sin(r) - data = MyData.fromts(key=csp.const(key), angle=csp.cast_int_to_float(angle), radians=r, sin=s, timestamp=times(snap)) + data = MyData.fromts( + key=csp.const(key), angle=csp.cast_int_to_float(angle), radians=r, sin=s, timestamp=times(snap) + ) all_structs.append(data) data = csp.flatten(all_structs) From a3e019120493e022e653d0f3f03e0c1fc05a54ee Mon Sep 17 00:00:00 2001 From: Sorin Vatasoiu Date: Thu, 22 Feb 2024 12:26:40 -0500 Subject: [PATCH 4/5] workaround for gcc c++20 bogus warnings Signed-off-by: Tim Paine --- cpp/csp/core/Exception.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/csp/core/Exception.h b/cpp/csp/core/Exception.h index af5512fd..d03f863b 100644 --- a/cpp/csp/core/Exception.h +++ b/cpp/csp/core/Exception.h @@ -27,7 +27,7 @@ class Exception : public std::exception m_full = m_file + ":" + m_function + ":" + std::to_string( m_line ) + ":"; m_full += m_exType + ": " + m_description; if( includeBacktrace && m_backtracesize > 0 ) - m_full += "\n" + backtraceString(); + m_full += '\n' + backtraceString(); return m_full; } const std::string & description() const noexcept { return m_description; } From cfcac949809fc688d2f0e28ad90ad2747a904146 Mon Sep 17 00:00:00 2001 From: Tim Paine Date: Thu, 22 Feb 2024 13:18:10 -0500 Subject: [PATCH 5/5] Ensure symphony is locking room mapper in get_room_name Signed-off-by: Tim Paine --- csp/adapters/symphony.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/csp/adapters/symphony.py b/csp/adapters/symphony.py index b0747c14..782d1008 100644 --- a/csp/adapters/symphony.py +++ b/csp/adapters/symphony.py @@ -192,13 +192,14 @@ def get_room_id(self, room_name): return room_id def get_room_name(self, room_id): - if room_id in self._id_to_name: - return self._id_to_name[room_id] - else: - room_name = _get_room_name(room_id, self._room_info_url, self._header) - self._name_to_id[room_name] = room_id - self._id_to_name[room_id] = room_name - return room_name + with self._lock: + if room_id in self._id_to_name: + return self._id_to_name[room_id] + else: + room_name = _get_room_name(room_id, self._room_info_url, self._header) + self._name_to_id[room_name] = room_id + self._id_to_name[room_id] = room_name + return room_name def set_im_id(self, user, id): with self._lock: