Skip to content

Commit e29f77b

Browse files
robambalurobambalu
authored
Merge pull request #138 from Point72/bugfix/struct_deepcopy_on_init
fixes #69 - deepcopy struct default values on init. Also includes th…
2 parents 0f5402a + e5ceaa2 commit e29f77b

6 files changed

Lines changed: 127 additions & 74 deletions

File tree

‎cpp/csp/engine/Struct.cpp‎

Lines changed: 24 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
namespace csp
66
{
77

8-
StructField::StructField( CspTypePtr type, const std::string & fieldname,
8+
StructField::StructField( CspTypePtr type, const std::string & fieldname,
99
size_t size, size_t alignment ) :
1010
m_fieldname( fieldname ),
1111
m_offset( 0 ),
@@ -18,15 +18,15 @@ StructField::StructField( CspTypePtr type, const std::string & fieldname,
1818
{
1919
}
2020

21-
/* StructMeta
21+
/* StructMeta
2222
2323
A note on member layout. Meta will order objects in the following order:
2424
- non-native fields ( ie PyObjectPtr for the python dialect )
2525
- native fields sorted in order
2626
- set/unset bitmask bytes ( 1 byte per 8 fields )
2727
2828
Derived structs will simply append to the layout of the base struct, properly padding between classes to align
29-
its fields properly.
29+
its fields properly.
3030
This layout is imposed on Struct instances. Since Struct needs refcount and meta * fields, for convenience they are stored
3131
*before* a Struct's "this" pointer as hidden data. This way struct ptrs can be passed into StructMeta without
3232
and adjustments required for the hidden fields
@@ -47,7 +47,7 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields,
4747
//decided to place them at the start cause they are most likely size of ptr or greater
4848

4949
m_fieldnames.reserve( m_fields.size() );
50-
for( size_t i = 0; i < m_fields.size(); i++ )
50+
for( size_t i = 0; i < m_fields.size(); i++ )
5151
m_fieldnames.emplace_back( m_fields[i] -> fieldname() );
5252

5353
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,
9393
m_isFullyNative = m_isPartialNative && ( m_base ? m_base -> isNative() : true );
9494

9595
//Setup masking bits for our fields
96-
//NOTE we can be more efficient by sticking masks into any potential alignment gaps, dont want to spend time on it
96+
//NOTE we can be more efficient by sticking masks into any potential alignment gaps, dont want to spend time on it
9797
//at this point
9898
m_maskSize = !m_fields.empty() ? 1 + ( ( m_fields.size() - 1 ) / 8 ) : 0;
9999
m_size = offset + m_maskSize;
@@ -116,7 +116,7 @@ StructMeta::StructMeta( const std::string & name, const Fields & fields,
116116
{
117117
m_fields.insert( m_fields.begin(), m_base -> m_fields.begin(), m_base -> m_fields.end() );
118118
m_fieldnames.insert( m_fieldnames.begin(), m_base -> m_fieldnames.begin(), m_base -> m_fieldnames.end() );
119-
119+
120120
m_firstPartialField = m_base -> m_fields.size();
121121
m_firstNativePartialField += m_base -> m_fields.size();
122122
m_fieldMap = m_base -> m_fieldMap;
@@ -145,7 +145,7 @@ Struct * StructMeta::createRaw() const
145145
initialize( s );
146146

147147
if( m_default )
148-
s -> copyFrom( m_default.get() ); //TODO change to deepcopy after this fix is released
148+
s -> deepcopyFrom( m_default.get() );
149149

150150
return s;
151151
}
@@ -212,7 +212,7 @@ void StructMeta::initialize( Struct * s ) const
212212
}
213213

214214
memset( reinterpret_cast<std::byte*>(s) + m_nativeStart, 0, partialNativeSize() );
215-
215+
216216
if( !m_isPartialNative )
217217
{
218218
for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx )
@@ -221,7 +221,7 @@ void StructMeta::initialize( Struct * s ) const
221221
static_cast<NonNativeStructField*>( field ) -> initialize( s );
222222
}
223223
}
224-
224+
225225
if( m_base )
226226
m_base -> initialize( s );
227227
}
@@ -231,9 +231,9 @@ void StructMeta::copyFrom( const Struct * src, Struct * dest )
231231
if( unlikely( src == dest ) )
232232
return;
233233

234-
if( dest -> meta() != src -> meta() &&
234+
if( dest -> meta() != src -> meta() &&
235235
!StructMeta::isDerivedType( src -> meta(), dest -> meta() ) )
236-
CSP_THROW( TypeError, "Attempting to copy from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name()
236+
CSP_THROW( TypeError, "Attempting to copy from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name()
237237
<< "'. copy_from may only be used to copy from same type or derived types" );
238238

239239
dest -> meta() -> copyFromImpl( src, dest, false );
@@ -280,7 +280,7 @@ void StructMeta::copyFromImpl( const Struct * src, Struct * dest, bool deepcopy
280280
//note that partialNative will include the mask bytes - this sets the native part and the mask
281281
memcpy( reinterpret_cast<std::byte*>(dest) + m_nativeStart, reinterpret_cast<const std::byte*>(src) + m_nativeStart,
282282
partialNativeSize() );
283-
283+
284284
if( m_base )
285285
m_base -> copyFromImpl( src, dest, deepcopy );
286286
}
@@ -291,13 +291,13 @@ void StructMeta::updateFrom( const Struct * src, Struct * dest )
291291
if( unlikely( src == dest ) )
292292
return;
293293

294-
if( dest -> meta() != src -> meta() &&
294+
if( dest -> meta() != src -> meta() &&
295295
!StructMeta::isDerivedType( src -> meta(), dest -> meta() ) )
296-
CSP_THROW( TypeError, "Attempting to update from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name()
296+
CSP_THROW( TypeError, "Attempting to update from struct type '" << src -> meta() -> name() << "' to struct type '" << dest -> meta() -> name()
297297
<< "'. update_from may only be used to update from same type or derived types" );
298298

299299
dest -> meta() -> updateFromImpl( src, dest );
300-
}
300+
}
301301

302302
void StructMeta::updateFromImpl( const Struct * src, Struct * dest ) const
303303
{
@@ -328,7 +328,7 @@ bool StructMeta::isEqual( const Struct * x, const Struct * y ) const
328328

329329
//Note the curent use of memcpy for native types. This can cause issues on double comparisons
330330
//esp if expecting NaN == NaN to be false, and when comparing -0.0 to +0.0.. may want to revisit
331-
//We we do we may as well remove the basepadding copy
331+
//We we do we may as well remove the basepadding copy
332332
if( isNative() )
333333
return memcmp( x, y, size() ) == 0;
334334

@@ -341,7 +341,7 @@ bool StructMeta::isEqual( const Struct * x, const Struct * y ) const
341341
for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx )
342342
{
343343
auto * field = m_fields[ idx ].get();
344-
344+
345345
if( field -> isSet( x ) != field -> isSet( y ) )
346346
return false;
347347

@@ -386,19 +386,19 @@ size_t StructMeta::hash( const Struct * x ) const
386386
"Exceeded max recursion depth of " << MAX_RECURSION_DEPTH << " in " << name() << "::hash(), cannot hash cyclic data structure" );
387387

388388
hash ^= csp::hash::hash_bytes( x + m_nativeStart, partialNativeSize() );
389-
389+
390390
if( !m_isPartialNative )
391391
{
392392
for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx )
393393
{
394394
auto * field = m_fields[ idx ].get();
395-
395+
396396
//we dont incorporate unset fields, bitmask will cover them
397397
if( field -> isSet( x ) )
398398
hash ^= static_cast<NonNativeStructField*>( field ) -> hash( x );
399399
}
400400
}
401-
401+
402402
if( m_base )
403403
hash ^= std::hash<uint64_t>()( (uint64_t ) m_base.get() ) ^ m_base -> hash( x );
404404

@@ -414,18 +414,18 @@ void StructMeta::clear( Struct * s ) const
414414
}
415415

416416
memset( reinterpret_cast<std::byte*>(s) + m_nativeStart, 0, partialNativeSize() );
417-
417+
418418
if( !m_isPartialNative )
419419
{
420420
for( size_t idx = m_firstPartialField; idx < m_firstNativePartialField; ++idx )
421421
{
422422
auto * field = m_fields[ idx ].get();
423-
423+
424424
if( field -> isSet( s ) )
425425
static_cast<NonNativeStructField*>( field ) -> clearValue( s );
426426
}
427427
}
428-
428+
429429
if( m_base )
430430
m_base -> clear( s );
431431
}
@@ -466,7 +466,7 @@ void StructMeta::destroy( Struct * s ) const
466466
static_cast<NonNativeStructField*>( field ) -> destroy( s );
467467
}
468468
}
469-
469+
470470
if( m_base )
471471
m_base -> destroy( s );
472472
}

‎cpp/csp/python/PyCspType.cpp‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
#include <csp/python/PyCspType.h>
2+
#include <csp/python/PyStruct.h>
23
#include <csp/python/Conversions.h>
34
#include <Python.h>
45

@@ -25,14 +26,14 @@ DialectGenericType::DialectGenericType( const DialectGenericType &rhs )
2526

2627
DialectGenericType::DialectGenericType( DialectGenericType &&rhs )
2728
{
28-
new( this ) csp::python::PyObjectPtr( reinterpret_cast<const csp::python::PyObjectPtr &&>(rhs) );
29+
new( this ) csp::python::PyObjectPtr( reinterpret_cast<csp::python::PyObjectPtr &&>(rhs) );
2930
}
3031

3132
DialectGenericType DialectGenericType::deepcopy() const
3233
{
33-
static PyObject *pyDeepcopy = PyObject_GetAttrString( PyImport_ImportModule( "copy" ), "deepcopy" );
34-
PyObject * pyVal = PyObject_CallFunction( pyDeepcopy, "O", python::toPythonBorrowed( *this ) );
35-
return DialectGenericType( reinterpret_cast<const DialectGenericType &&>( std::move( csp::python::PyObjectPtr::own( pyVal ) ) ) );
34+
static PyObject * pyDeepcopy = PyObject_GetAttrString( PyImport_ImportModule( "copy" ), "deepcopy" );
35+
PyObject * pyVal = PyObject_CallFunction( pyDeepcopy, "(O)", python::toPythonBorrowed( *this ) );
36+
return DialectGenericType( reinterpret_cast<DialectGenericType &&>( std::move( csp::python::PyObjectPtr::check( pyVal ) ) ) );
3637
}
3738

3839
DialectGenericType &DialectGenericType::operator=( const DialectGenericType &rhs )
@@ -43,7 +44,7 @@ DialectGenericType &DialectGenericType::operator=( const DialectGenericType &rhs
4344

4445
DialectGenericType &DialectGenericType::operator=( DialectGenericType &&rhs )
4546
{
46-
*reinterpret_cast<csp::python::PyObjectPtr *>(this) = reinterpret_cast<const csp::python::PyObjectPtr &&>(rhs);
47+
*reinterpret_cast<csp::python::PyObjectPtr *>(this) = std::move( reinterpret_cast<csp::python::PyObjectPtr &&>(rhs) );
4748
return *this;
4849
}
4950

‎cpp/csp/python/PyStruct.cpp‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -169,9 +169,9 @@ static PyObject * PyStructMeta_new( PyTypeObject *subtype, PyObject *args, PyObj
169169

170170
/*back reference to the struct type that will be accessible on the csp struct -> meta()
171171
DialectStructMeta will hold a borrowed reference to the type to avoid a circular dep
172-
172+
173173
This is the layout of references between all these types
174-
StructMeta (shared_ptr) <-------- strong ref
174+
StructMeta (shared_ptr) <-------- strong ref
175175
| |
176176
DialectStructMeta ---> weak ref to PyStructMeta ( the PyType )
177177
/\ /\
@@ -671,10 +671,10 @@ PyObject * PyStruct_new( PyTypeObject * type, PyObject *args, PyObject *kwds )
671671
if( ! ( (PyStructMeta * ) type ) -> structMeta )
672672
CSP_THROW( TypeError, "csp.Struct cannot be instantiated" );
673673

674-
PyStruct * pystruct = ( PyStruct * ) type -> tp_alloc( type, 0 );
675-
676674
StructPtr struct_ = ( (PyStructMeta * ) type ) -> structMeta -> create();
677675

676+
PyStruct * pystruct = ( PyStruct * ) type -> tp_alloc( type, 0 );
677+
678678
//assign dialectptr, but we DO NOT incref the instance on the struct
679679
struct_ -> setDialectPtr( pystruct );
680680

@@ -845,16 +845,24 @@ int PyStruct_setattro( PyStruct * self, PyObject * attr, PyObject * value )
845845

846846
PyObject * PyStruct_copy( PyStruct * self )
847847
{
848+
CSP_BEGIN_METHOD;
848849
PyObject * copy = self -> ob_type -> tp_alloc( self -> ob_type, 0 );
849850
new ( copy ) PyStruct( self -> struct_ -> copy() );
850851
return copy;
852+
CSP_RETURN_NULL;
851853
}
852854

853855
PyObject * PyStruct_deepcopy( PyStruct * self )
854856
{
855-
PyObject * deepcopy = self -> ob_type -> tp_alloc( self -> ob_type, 0 );
856-
new ( deepcopy ) PyStruct( self -> struct_ -> deepcopy() );
857-
return deepcopy;
857+
CSP_BEGIN_METHOD;
858+
//Note that once tp_alloc is called, the object will get added to GC
859+
//deepcopy traversal may kick in a GC collect, so we have to call that first before the PyStruct is created
860+
//of it may traverse a partially consturcted object and crash
861+
auto deepcopy = self -> struct_ -> deepcopy();
862+
PyObject * pyDeepcopy = self -> ob_type -> tp_alloc( self -> ob_type, 0 );
863+
new ( pyDeepcopy ) PyStruct( deepcopy );
864+
return pyDeepcopy;
865+
CSP_RETURN_NULL;
858866
}
859867

860868
PyObject * PyStruct_clear( PyStruct * self )

‎csp/impl/struct.py‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,9 @@ def __getstate__(self):
195195
def __setstate__(self, state):
196196
self.update(**state)
197197

198+
def __deepcopy__(self, memodict={}):
199+
return self.deepcopy()
200+
198201
def __dir__(self):
199202
return self.__full_metadata_typed__.keys()
200203

‎csp/impl/wiring/numba_node.py‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,17 +67,17 @@ def _create(self, engine, memo):
6767
exposed_utility_values = {}
6868
cache_key = [self._impl]
6969
for (ts_idx, basket_idx), input in self.ts_inputs():
70-
exposed_utility_values[NumbaNodeParser.get_ts_input_value_getter_name(ts_idx)] = (
71-
NumbaTSTypedFunctionResolver.get_value_getter_function(input.tstype.typ)
72-
)
70+
exposed_utility_values[
71+
NumbaNodeParser.get_ts_input_value_getter_name(ts_idx)
72+
] = NumbaTSTypedFunctionResolver.get_value_getter_function(input.tstype.typ)
7373
cache_key.append(input.tstype)
7474

7575
for output in self._outputs:
7676
assert output.kind.is_single_ts()
7777
ts_idx = output.ts_idx
78-
exposed_utility_values[NumbaNodeParser.get_ts_out_value_return_name(ts_idx)] = (
79-
NumbaTSTypedFunctionResolver.get_value_returner_function(output.typ.typ)
80-
)
78+
exposed_utility_values[
79+
NumbaNodeParser.get_ts_out_value_return_name(ts_idx)
80+
] = NumbaTSTypedFunctionResolver.get_value_returner_function(output.typ.typ)
8181

8282
numba_scalar_types = []
8383
for i in self._signature.raw_inputs():

0 commit comments

Comments
 (0)