From 5b5c7d41b4e493df8763a7bc4275eaa1ce93313e Mon Sep 17 00:00:00 2001 From: Seth Hillbrand Date: Fri, 11 Mar 2022 13:46:21 -0800 Subject: [PATCH] Break up buildConnectionGraph for debugging Needed to find bottlenecks in fns, so break out individual sections of the massive function for easier understanding. buildItemSubgraphs (one section of the previous function) would build millions of connections that were never used as stacked pins created X! connections. Also tested using sets instead of lists and keeping unique lists to avoid flagging but none of these were as performant as using flags to remember which items had already been processed. Fixes https://gitlab.com/kicad/code/kicad/issues/10974 (cherry picked from commit 17b1b68ac78dafdac2c40b2eeef2c908c802d07d) --- eeschema/connection_graph.cpp | 107 +++++++++++++++++++++++++--------- eeschema/connection_graph.h | 27 +++++++++ eeschema/sch_item.cpp | 13 +++-- eeschema/sch_label.cpp | 15 +++-- 4 files changed, 126 insertions(+), 36 deletions(-) diff --git a/eeschema/connection_graph.cpp b/eeschema/connection_graph.cpp index 945ef187b5..e3741bf336 100644 --- a/eeschema/connection_graph.cpp +++ b/eeschema/connection_graph.cpp @@ -708,19 +708,7 @@ void CONNECTION_GRAPH::updateItemConnectivity( const SCH_SHEET_PATH& aSheet, } -// TODO(JE) This won't give the same subgraph IDs (and eventually net/graph codes) -// to the same subgraph necessarily if it runs over and over again on the same -// sheet. We need: -// -// a) a cache of net/bus codes, like used before -// b) to persist the CONNECTION_GRAPH globally so the cache is persistent, -// c) some way of trying to avoid changing net names. so we should keep track -// of the previous driver of a net, and if it comes down to choosing between -// equally-prioritized drivers, choose the one that already exists as a driver -// on some portion of the items. - - -void CONNECTION_GRAPH::buildConnectionGraph() +void CONNECTION_GRAPH::buildItemSubGraphs() { // Recache all bus aliases for later use wxCHECK_RET( m_schematic, "Connection graph cannot be built without schematic pointer" ); @@ -754,21 +742,25 @@ void CONNECTION_GRAPH::buildConnectionGraph() connection->SetSubgraphCode( subgraph->m_code ); m_item_to_subgraph_map[item] = subgraph; - std::list members; + std::list memberlist; auto get_items = [&]( SCH_ITEM* aItem ) -> bool { SCH_CONNECTION* conn = aItem->GetOrInitConnection( sheet, this ); + bool unique = !( aItem->GetFlags() & CANDIDATE ); - return ( conn->SubgraphCode() == 0 ); + aItem->SetFlags( CANDIDATE ); + + return ( unique && conn && ( conn->SubgraphCode() == 0 ) ); }; + std::copy_if( item->ConnectedItems( sheet ).begin(), item->ConnectedItems( sheet ).end(), - std::back_inserter( members ), get_items ); + std::back_inserter( memberlist ), get_items ); - for( SCH_ITEM* connected_item : members ) + for( SCH_ITEM* connected_item : memberlist ) { if( connected_item->Type() == SCH_NO_CONNECT_T ) subgraph->m_no_connect = connected_item; @@ -782,24 +774,26 @@ void CONNECTION_GRAPH::buildConnectionGraph() connected_conn->SetSubgraphCode( subgraph->m_code ); m_item_to_subgraph_map[connected_item] = subgraph; subgraph->AddItem( connected_item ); + SCH_ITEM_SET citemset = connected_item->ConnectedItems( sheet ); - std::copy_if( connected_item->ConnectedItems( sheet ).begin(), - connected_item->ConnectedItems( sheet ).end(), - std::back_inserter( members ), get_items ); + std::copy_if( citemset.begin(), citemset.end(), + std::back_inserter( memberlist ), get_items ); } } + for( SCH_ITEM* connected_item : memberlist ) + connected_item->ClearFlags( CANDIDATE ); + subgraph->m_dirty = true; m_subgraphs.push_back( subgraph ); } } } - /** - * TODO(JE): Net codes are non-deterministic. Fortunately, they are also not really used for - * anything. We should consider removing them entirely and just using net names everywhere. - */ +} +void CONNECTION_GRAPH::resolveAllDrivers() +{ // Resolve drivers for subgraphs and propagate connectivity info // We don't want to spin up a new thread for fewer than 8 nets (overhead costs) @@ -879,7 +873,11 @@ void CONNECTION_GRAPH::buildConnectionGraph() { return candidate->m_driver; } ); +} + +void CONNECTION_GRAPH::collectAllDriverValues() +{ // Check for subgraphs with the same net name but only weak drivers. // For example, two wires that are both connected to hierarchical // sheet pins that happen to have the same name, but are not the same. @@ -934,7 +932,11 @@ void CONNECTION_GRAPH::buildConnectionGraph() } } } +} + +void CONNECTION_GRAPH::generateInvisiblePinSubGraphs() +{ // Generate subgraphs for invisible power pins. These will be merged with other subgraphs // on the same sheet in the next loop. @@ -954,7 +956,7 @@ void CONNECTION_GRAPH::buildConnectionGraph() SCH_CONNECTION* connection = pin->GetOrInitConnection( sheet, this ); // If this pin already has a subgraph, don't need to process - if( connection->SubgraphCode() > 0 ) + if( !connection || connection->SubgraphCode() > 0 ) continue; connection->SetName( pin->GetShownName() ); @@ -991,7 +993,11 @@ void CONNECTION_GRAPH::buildConnectionGraph() connection->SetSubgraphCode( subgraph->m_code ); } +} + +void CONNECTION_GRAPH::processSubGraphs() +{ // Here we do all the local (sheet) processing of each subgraph, including assigning net // codes, merging subgraphs together that use label connections, etc. @@ -1318,6 +1324,49 @@ void CONNECTION_GRAPH::buildConnectionGraph() subgraph->m_driver_connection->Name() ); } +} + + +// TODO(JE) This won't give the same subgraph IDs (and eventually net/graph codes) +// to the same subgraph necessarily if it runs over and over again on the same +// sheet. We need: +// +// a) a cache of net/bus codes, like used before +// b) to persist the CONNECTION_GRAPH globally so the cache is persistent, +// c) some way of trying to avoid changing net names. so we should keep track +// of the previous driver of a net, and if it comes down to choosing between +// equally-prioritized drivers, choose the one that already exists as a driver +// on some portion of the items. + + +void CONNECTION_GRAPH::buildConnectionGraph() +{ + // Recache all bus aliases for later use + wxCHECK_RET( m_schematic, wxT( "Connection graph cannot be built without schematic pointer" ) ); + + SCH_SHEET_LIST all_sheets = m_schematic->GetSheets(); + + for( unsigned i = 0; i < all_sheets.size(); i++ ) + { + for( const auto& alias : all_sheets[i].LastScreen()->GetBusAliases() ) + m_bus_alias_cache[ alias->GetName() ] = alias; + } + + buildItemSubGraphs(); + + /** + * TODO(JE): Net codes are non-deterministic. Fortunately, they are also not really used for + * anything. We should consider removing them entirely and just using net names everywhere. + */ + + resolveAllDrivers(); + + collectAllDriverValues(); + + generateInvisiblePinSubGraphs(); + + processSubGraphs(); + // Absorbed subgraphs should no longer be considered alg::delete_if( m_driver_subgraphs, [&]( const CONNECTION_SUBGRAPH* candidate ) -> bool { @@ -1340,7 +1389,13 @@ void CONNECTION_GRAPH::buildConnectionGraph() m_sheet_to_subgraphs_map[ subgraph->m_sheet ].emplace_back( subgraph ); // Update item connections at this point so that neighbor propagation works - nextSubgraph.store( 0 ); + std::atomic nextSubgraph( 0 ); + + // We don't want to spin up a new thread for fewer than 8 nets (overhead costs) + size_t parallelThreadCount = std::min( std::thread::hardware_concurrency(), + ( m_subgraphs.size() + 3 ) / 4 ); + + std::vector> returns( parallelThreadCount ); auto preliminaryUpdateTask = [&]() -> size_t diff --git a/eeschema/connection_graph.h b/eeschema/connection_graph.h index f8ac81e5eb..e3be9469c0 100644 --- a/eeschema/connection_graph.h +++ b/eeschema/connection_graph.h @@ -370,6 +370,33 @@ private: */ void buildConnectionGraph(); + /** + * Generates individual item subgraphs on a per-sheet basis + */ + void buildItemSubGraphs(); + + /** + * Finds all subgraphs in the connection graph and calls ResolveDrivers() in + * parallel + */ + void resolveAllDrivers(); + + /** + * Maps the driver values for each subgraph + */ + void collectAllDriverValues(); + + /** + * Iterate through the invisible power pins to collect the global labels + * as drivers + */ + void generateInvisiblePinSubGraphs(); + + /** + * Process all subgraphs to assign netcodes and merge subgraphs based on labels + */ + void processSubGraphs(); + /** * Helper to assign a new net code to a connection * diff --git a/eeschema/sch_item.cpp b/eeschema/sch_item.cpp index 25541f85b1..f2780da26c 100644 --- a/eeschema/sch_item.cpp +++ b/eeschema/sch_item.cpp @@ -183,11 +183,13 @@ SCH_ITEM_SET& SCH_ITEM::ConnectedItems( const SCH_SHEET_PATH& aSheet ) void SCH_ITEM::AddConnectionTo( const SCH_SHEET_PATH& aSheet, SCH_ITEM* aItem ) { - // The vector elements are small, so reserve 1k at a time to prevent re-allocations - if( m_connected_items[ aSheet ].capacity() == 0 ) - m_connected_items[ aSheet ].reserve( 1024 ); + SCH_ITEM_SET& set = m_connected_items[ aSheet ]; - m_connected_items[ aSheet ].emplace_back( aItem ); + // The vector elements are small, so reserve 1k at a time to prevent re-allocations + if( set.size() == set.capacity() ) + set.reserve( set.size() + 4096 ); + + set.emplace_back( aItem ); } @@ -217,6 +219,9 @@ SCH_CONNECTION* SCH_ITEM::InitializeConnection( const SCH_SHEET_PATH& aSheet, SCH_CONNECTION* SCH_ITEM::GetOrInitConnection( const SCH_SHEET_PATH& aSheet, CONNECTION_GRAPH* aGraph ) { + if( !IsConnectable() ) + return nullptr; + SetConnectivityDirty( false ); SCH_CONNECTION* connection = Connection( &aSheet ); diff --git a/eeschema/sch_label.cpp b/eeschema/sch_label.cpp index 8665322038..74186a01cc 100644 --- a/eeschema/sch_label.cpp +++ b/eeschema/sch_label.cpp @@ -217,12 +217,17 @@ bool SCH_LABEL_BASE::IsType( const KICAD_T aScanTypes[] ) const { if( *p == SCH_LABEL_LOCATE_ANY_T ) return true; + } + wxCHECK_MSG( Schematic(), false, wxT( "No parent SCHEMATIC set for SCH_LABEL!" ) ); + + const SCH_ITEM_SET& item_set = m_connected_items.at( Schematic()->CurrentSheet() ); + + for( const KICAD_T* p = aScanTypes; *p != EOT; ++p ) + { if( *p == SCH_LABEL_LOCATE_WIRE_T ) { - wxCHECK_MSG( Schematic(), false, "No parent SCHEMATIC set for SCH_LABEL!" ); - - for( SCH_ITEM* connection : m_connected_items.at( Schematic()->CurrentSheet() ) ) + for( SCH_ITEM* connection : item_set ) { if( connection->IsType( wireTypes ) ) return true; @@ -231,9 +236,7 @@ bool SCH_LABEL_BASE::IsType( const KICAD_T aScanTypes[] ) const if ( *p == SCH_LABEL_LOCATE_BUS_T ) { - wxCHECK_MSG( Schematic(), false, "No parent SCHEMATIC set for SCH_LABEL!" ); - - for( SCH_ITEM* connection : m_connected_items.at( Schematic()->CurrentSheet() ) ) + for( SCH_ITEM* connection : item_set ) { if( connection->IsType( busTypes ) ) return true;