From f3c15e3b29e926bfbe5cc354e25dac74d10bb053 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 | 180 ++++++++++++++++++++++++--------------------- eeschema/sch_component.h | 19 ++--- 2 files changed, 103 insertions(+), 96 deletions(-) diff --git a/eeschema/sch_component.cpp b/eeschema/sch_component.cpp index d951a8c86..1b154473c 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 ); @@ -297,6 +298,28 @@ wxString SCH_COMPONENT::GetAliasDocumentation() const } +void SCH_COMPONENT::UpdatePinCache() +{ + m_Pins.clear(); + + if( PART_SPTR part = m_part.lock() ) + { + 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() ); + } + } +} + + bool SCH_COMPONENT::Resolve( PART_LIBS* aLibs ) { // I've never been happy that the actual individual PART_LIB is left up to @@ -376,6 +399,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 +410,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 +443,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 +454,9 @@ 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; } } @@ -1488,67 +1519,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 +1618,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 +1838,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 +1885,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..7ded7f814 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,11 @@ public: int GetUnit() const { return m_unit; } /** + * Updates the local cache of pin positions + */ + void UpdatePinCache(); + + /** * Change the unit number to \a aUnit * * This has meaning only for symbols made up of multiple units per package. @@ -530,19 +536,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. -- 2.11.0