From de77f155e16e149f53fd9b1fbb6e949c8a63609a Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Sun, 10 Dec 2017 19:28:34 -0800 Subject: [PATCH] Optimizing speed of sch_component pin references Pins are weak_ptrs to the library, so they require a lock() before accessing. This imposes a performance hit on fast loops that access the pin list repeatedly. This patch caches the pin position locally for each component, updating only when needed. Fixes: lp:1737363 * https://bugs.launchpad.net/kicad/+bug/1737363 --- eeschema/sch_component.cpp | 224 ++++++++++++++++++++++++++++----------------- eeschema/sch_component.h | 26 +++--- eeschema/sch_screen.cpp | 12 ++- 3 files changed, 161 insertions(+), 101 deletions(-) diff --git a/eeschema/sch_component.cpp b/eeschema/sch_component.cpp index d951a8c86..32414f96f 100644 --- a/eeschema/sch_component.cpp +++ b/eeschema/sch_component.cpp @@ -172,6 +172,7 @@ SCH_COMPONENT::SCH_COMPONENT( const SCH_COMPONENT& aComponent ) : m_convert = aComponent.m_convert; m_lib_id = aComponent.m_lib_id; m_part = aComponent.m_part; + m_Pins = aComponent.m_Pins; SetTimeStamp( aComponent.m_TimeStamp ); @@ -376,6 +377,7 @@ void SCH_COMPONENT::ResolveAll( const SCH_COLLECTOR& aComponents, PART_LIBS* aLi SCH_COMPONENT* cmp = cmp_list[ii]; curr_libid = cmp->m_lib_id; cmp->Resolve( aLibs ); + cmp->UpdatePinCache(); // Propagate the m_part pointer to other members using the same lib_id for( unsigned jj = ii+1; jj < cmp_list.size (); ++jj ) @@ -386,6 +388,9 @@ void SCH_COMPONENT::ResolveAll( const SCH_COLLECTOR& aComponents, PART_LIBS* aLi break; next_cmp->m_part = cmp->m_part; + + // Propagate the pin cache vector as well + next_cmp->m_Pins = cmp->m_Pins; ii = jj; } } @@ -416,6 +421,7 @@ void SCH_COMPONENT::ResolveAll( const SCH_COLLECTOR& aComponents, SYMBOL_LIB_TAB SCH_COMPONENT* cmp = cmp_list[ii]; curr_libid = cmp->m_lib_id; cmp->Resolve( aLibTable, aCacheLib ); + cmp->UpdatePinCache(); // Propagate the m_part pointer to other members using the same lib_id for( unsigned jj = ii+1; jj < cmp_list.size (); ++jj ) @@ -426,6 +432,75 @@ void SCH_COMPONENT::ResolveAll( const SCH_COLLECTOR& aComponents, SYMBOL_LIB_TAB break; next_cmp->m_part = cmp->m_part; + + // Propagate the pin cache vector as well + next_cmp->m_Pins = cmp->m_Pins; + ii = jj; + } + } +} + + +void SCH_COMPONENT::UpdatePinCache() +{ + if( PART_SPTR part = m_part.lock() ) + { + m_Pins.clear(); + for( LIB_PIN* pin = part->GetNextPin(); pin; pin = part->GetNextPin( pin ) ) + { + wxASSERT( pin->Type() == LIB_PIN_T ); + + if( pin->GetUnit() && m_unit && ( m_unit != pin->GetUnit() ) ) + continue; + + if( pin->GetConvert() && m_convert && ( m_convert != pin->GetConvert() ) ) + continue; + + m_Pins.push_back( pin->GetPosition() ); + } + } +} + + +void SCH_COMPONENT::UpdateAllPinCaches( const SCH_COLLECTOR& aComponents ) +{ + // Usually, many components use the same part lib. + // to avoid too long calculation time the list of components is grouped + // and once the lib part is found for one member of a group, it is also + // set for all other members of this group + std::vector cmp_list; + + // build the cmp list. + for( int i = 0; i < aComponents.GetCount(); ++i ) + { + SCH_COMPONENT* cmp = dynamic_cast( aComponents[i] ); + wxASSERT( cmp ); + + if( cmp ) // cmp == NULL should not occur. + cmp_list.push_back( cmp ); + } + + // sort it by lib part. Cmp will be grouped by same lib part. + std::sort( cmp_list.begin(), cmp_list.end(), sort_by_libid ); + + LIB_ID curr_libid; + + for( unsigned ii = 0; ii < cmp_list.size (); ++ii ) + { + SCH_COMPONENT* cmp = cmp_list[ii]; + curr_libid = cmp->m_lib_id; + cmp->UpdatePinCache(); + + // Propagate the m_Pins vector to other members using the same lib_id + for( unsigned jj = ii+1; jj < cmp_list.size (); ++jj ) + { + SCH_COMPONENT* next_cmp = cmp_list[jj]; + + if( curr_libid != next_cmp->m_lib_id ) + break; + + // Propagate the pin cache vector as well + next_cmp->m_Pins = cmp->m_Pins; ii = jj; } } @@ -1488,67 +1563,64 @@ void SCH_COMPONENT::GetEndPoints( std::vector & aItemList ) } -bool SCH_COMPONENT::IsPinDanglingStateChanged( std::vector &aItemList, - LIB_PINS& aLibPins, unsigned aPin ) +bool SCH_COMPONENT::IsDanglingStateChanged( std::vector& aItemList ) { - bool previousState; - - if( aPin < m_isDangling.size() ) - { - previousState = m_isDangling[aPin]; - m_isDangling[aPin] = true; - } - else - { - previousState = true; - m_isDangling.push_back( true ); - } - - wxPoint pin_position = GetPinPhysicalPosition( aLibPins[aPin] ); + bool changed = false; - for( DANGLING_END_ITEM& each_item : aItemList ) + for( size_t i = 0; i < m_Pins.size(); ++i ) { - // Some people like to stack pins on top of each other in a symbol to indicate - // internal connection. While technically connected, it is not particularly useful - // to display them that way, so skip any pins that are in the same symbol as this - // one. - if( each_item.GetParent() == this ) - continue; + bool previousState; + wxPoint pos = m_transform.TransformCoordinate( m_Pins[ i ] ) + m_Pos; - switch( each_item.GetType() ) + if( i < m_isDangling.size() ) { - case PIN_END: - case LABEL_END: - case SHEET_LABEL_END: - case WIRE_START_END: - case WIRE_END_END: - case NO_CONNECT_END: - case JUNCTION_END: - if( pin_position == each_item.GetPosition() ) - m_isDangling[aPin] = false; - break; - default: - break; + previousState = m_isDangling[ i ]; + m_isDangling[ i ] = true; + } + else + { + previousState = true; + m_isDangling.push_back( true ); } - if( !m_isDangling[aPin] ) - break; - } - return previousState != m_isDangling[aPin]; -} + for( DANGLING_END_ITEM& each_item : aItemList ) + { + // Some people like to stack pins on top of each other in a symbol to indicate + // internal connection. While technically connected, it is not particularly useful + // to display them that way, so skip any pins that are in the same symbol as this + // one. + if( each_item.GetParent() == this ) + continue; + switch( each_item.GetType() ) + { + case PIN_END: + case LABEL_END: + case SHEET_LABEL_END: + case WIRE_START_END: + case WIRE_END_END: + case NO_CONNECT_END: + case JUNCTION_END: -bool SCH_COMPONENT::IsDanglingStateChanged( std::vector& aItemList ) -{ - bool changed = false; - LIB_PINS libPins; - if( PART_SPTR part = m_part.lock() ) - part->GetPins( libPins, m_unit, m_convert ); - for( size_t i = 0; i < libPins.size(); ++i ) - { - if( IsPinDanglingStateChanged( aItemList, libPins, i ) ) - changed = true; + if( pos == each_item.GetPosition() ) + m_isDangling[ i ] = false; + + break; + + default: + break; + } + + if( !m_isDangling[ i ] ) + break; + } + + changed = ( changed || ( previousState != m_isDangling[ i ] ) ); } + + while( m_isDangling.size() > m_Pins.size() ) + m_isDangling.pop_back(); + return changed; } @@ -1590,37 +1662,31 @@ bool SCH_COMPONENT::IsSelectStateChanged( const wxRect& aRect ) void SCH_COMPONENT::GetConnectionPoints( std::vector< wxPoint >& aPoints ) const { + for( auto pin : m_Pins ) + aPoints.push_back( m_transform.TransformCoordinate( pin ) + m_Pos ); +} + + +LIB_ITEM* SCH_COMPONENT::GetDrawItem( const wxPoint& aPosition, KICAD_T aType ) +{ if( PART_SPTR part = m_part.lock() ) { - for( LIB_PIN* pin = part->GetNextPin(); pin; pin = part->GetNextPin( pin ) ) + + m_Pins.clear(); + + for( LIB_PIN* pin = part->GetNextPin(); pin; pin = part->GetNextPin( pin ) ) { - wxCHECK_RET( pin->Type() == LIB_PIN_T, - wxT( "GetNextPin() did not return a pin object. Bad programmer!" ) ); + wxASSERT( pin->Type() == LIB_PIN_T ); - // Skip items not used for this part. - if( m_unit && pin->GetUnit() && ( pin->GetUnit() != m_unit ) ) + if( pin->GetUnit() && m_unit && ( m_unit != pin->GetUnit() ) ) continue; - if( m_convert && pin->GetConvert() && ( pin->GetConvert() != m_convert ) ) + if( pin->GetConvert() && m_convert && ( m_convert != pin->GetConvert() ) ) continue; - // Calculate the pin position relative to the component position and orientation. - aPoints.push_back( m_transform.TransformCoordinate( pin->GetPosition() ) + m_Pos ); + m_Pins.push_back( pin->GetPosition() ); } - } - else - { - wxCHECK_RET( 0, - wxT( "Cannot add connection points to list. Cannot find component <" ) + - GetLibId().GetLibItemName() + wxT( "> in any of the loaded libraries." ) ); - } -} - -LIB_ITEM* SCH_COMPONENT::GetDrawItem( const wxPoint& aPosition, KICAD_T aType ) -{ - if( PART_SPTR part = m_part.lock() ) - { // Calculate the position relative to the component. wxPoint libPosition = aPosition - m_Pos; @@ -1816,6 +1882,7 @@ SCH_ITEM& SCH_COMPONENT::operator=( const SCH_ITEM& aItem ) m_unit = c->m_unit; m_convert = c->m_convert; m_transform = c->m_transform; + m_Pins = c->m_Pins; m_PathsAndReferences = c->m_PathsAndReferences; @@ -1862,17 +1929,8 @@ bool SCH_COMPONENT::HitTest( const EDA_RECT& aRect, bool aContained, int aAccura bool SCH_COMPONENT::doIsConnected( const wxPoint& aPosition ) const { - std::vector< wxPoint > pts; - - GetConnectionPoints( pts ); - - for( size_t i = 0; i < pts.size(); i++ ) - { - if( pts[i] == aPosition ) - return true; - } - - return false; + wxPoint new_pos = m_transform.InverseTransform().TransformCoordinate( aPosition - m_Pos ); + return std::find( m_Pins.begin(), m_Pins.end(), new_pos ) != m_Pins.end(); } diff --git a/eeschema/sch_component.h b/eeschema/sch_component.h index 788223c86..cfd186fcf 100644 --- a/eeschema/sch_component.h +++ b/eeschema/sch_component.h @@ -93,6 +93,7 @@ private: PART_REF m_part; ///< points into the PROJECT's libraries to the LIB_PART for this component std::vector m_isDangling; ///< One isDangling per pin + std::vector m_Pins; AUTOPLACED m_fieldsAutoplaced; ///< indicates status of field autoplacement @@ -199,6 +200,18 @@ public: int GetUnit() const { return m_unit; } /** + * Updates the local cache of pin positions + */ + void UpdatePinCache(); + + /** + * Update the pin cache for all components in \a aComponents + * + * @param aComponents collector of components in screen + */ + static void UpdateAllPinCaches( const SCH_COLLECTOR& aComponents ); + + /** * Change the unit number to \a aUnit * * This has meaning only for symbols made up of multiple units per package. @@ -530,19 +543,6 @@ public: void GetEndPoints( std::vector& aItemList ) override; /** - * Test if the symbol's dangling state has changed for one given pin index. - * - * As a side effect, it actually updates the dangling status for that pin. - * - * @param aItemList is list of all #DANGLING_END_ITEM items to be tested. - * @param aLibPins is list of all the #LIB_PIN items in this symbol - * @param aPin is the index into \a aLibPins that identifies the pin to test - * @return true if the pin's state has changed. - */ - bool IsPinDanglingStateChanged( std::vector& aItemList, - LIB_PINS& aLibPins, unsigned aPin ); - - /** * Test if the component's dangling state has changed for all pins. * * As a side effect, actually update the dangling status for all pins. diff --git a/eeschema/sch_screen.cpp b/eeschema/sch_screen.cpp index d0c444a88..5ccbdabda 100644 --- a/eeschema/sch_screen.cpp +++ b/eeschema/sch_screen.cpp @@ -484,23 +484,25 @@ void SCH_SCREEN::UpdateSymbolLinks( bool aForce ) // Initialize or reinitialize the pointer to the LIB_PART for each component // found in m_drawList, but only if needed (change in lib or schematic) // therefore the calculation time is usually very low. - if( m_drawList.GetCount() ) { SYMBOL_LIB_TABLE* libs = Prj().SchSymbolLibTable(); int mod_hash = libs->GetModifyHash(); + SCH_TYPE_COLLECTOR c; + + c.Collect( GetDrawItems(), SCH_COLLECTOR::ComponentsOnly ); // Must we resolve? if( (m_modification_sync != mod_hash) || aForce ) { - SCH_TYPE_COLLECTOR c; - - c.Collect( GetDrawItems(), SCH_COLLECTOR::ComponentsOnly ); - SCH_COMPONENT::ResolveAll( c, *libs, Prj().SchLibs()->GetCacheLibrary() ); m_modification_sync = mod_hash; // note the last mod_hash } + // Resolving will update the pin caches but we must ensure that this happens + // even if the libraries don't change. + else + SCH_COMPONENT::UpdateAllPinCaches( c ); } } -- 2.11.0