From 758974f5aa4798ebf8ef9ebec9671bcb2f8b5433 Mon Sep 17 00:00:00 2001 From: Jeff Young Date: Thu, 6 Jun 2024 11:40:24 +0100 Subject: [PATCH] Performance for large hierarchies: sorting Cache page numbers during sort. Don't construct SCH_SHEET_PATH when reference will do. Don't construct SCH_SHEET_PATH when KIID_PATH will do. --- eeschema/sch_sheet.cpp | 28 +++++------------ eeschema/sch_sheet.h | 24 ++++++--------- eeschema/sch_sheet_path.cpp | 39 +++++++++++++++--------- eeschema/sch_sheet_path.h | 8 +++-- eeschema/tools/sch_find_replace_tool.cpp | 31 ++++++------------- 5 files changed, 57 insertions(+), 73 deletions(-) diff --git a/eeschema/sch_sheet.cpp b/eeschema/sch_sheet.cpp index 6993e31375..13ed2968ce 100644 --- a/eeschema/sch_sheet.cpp +++ b/eeschema/sch_sheet.cpp @@ -1426,25 +1426,22 @@ void SCH_SHEET::AddInstance( const SCH_SHEET_INSTANCE& aInstance ) } -bool SCH_SHEET::addInstance( const SCH_SHEET_PATH& aSheetPath ) +bool SCH_SHEET::addInstance( const KIID_PATH& aPath ) { - wxCHECK( aSheetPath.IsFullPath(), false ); - wxCHECK( !aSheetPath.Last() || ( aSheetPath.Last()->m_Uuid != m_Uuid ), false ); - for( const SCH_SHEET_INSTANCE& instance : m_instances ) { // if aSheetPath is found, nothing to do: - if( instance.m_Path == aSheetPath.Path() ) + if( instance.m_Path == aPath ) return false; } wxLogTrace( traceSchSheetPaths, wxT( "Adding instance `%s` to sheet `%s`." ), - aSheetPath.Path().AsString(), + aPath.AsString(), ( GetName().IsEmpty() ) ? wxString( wxT( "root" ) ) : GetName() ); SCH_SHEET_INSTANCE instance; - instance.m_Path = aSheetPath.Path(); + instance.m_Path = aPath; // This entry does not exist: add it with an empty page number. m_instances.emplace_back( instance ); @@ -1504,17 +1501,13 @@ const SCH_SHEET_INSTANCE& SCH_SHEET::GetRootInstance() const } -wxString SCH_SHEET::getPageNumber( const SCH_SHEET_PATH& aSheetPath ) const +wxString SCH_SHEET::getPageNumber( const KIID_PATH& aPath ) const { - wxCHECK( aSheetPath.IsFullPath(), wxEmptyString ); - wxCHECK( !aSheetPath.Last() || ( aSheetPath.Last()->m_Uuid != m_Uuid ), wxEmptyString ); - wxString pageNumber; - KIID_PATH path = aSheetPath.Path(); for( const SCH_SHEET_INSTANCE& instance : m_instances ) { - if( instance.m_Path == path ) + if( instance.m_Path == aPath ) { pageNumber = instance.m_PageNumber; break; @@ -1525,16 +1518,11 @@ wxString SCH_SHEET::getPageNumber( const SCH_SHEET_PATH& aSheetPath ) const } -void SCH_SHEET::setPageNumber( const SCH_SHEET_PATH& aSheetPath, const wxString& aPageNumber ) +void SCH_SHEET::setPageNumber( const KIID_PATH& aPath, const wxString& aPageNumber ) { - wxCHECK( aSheetPath.IsFullPath(), /* void */ ); - wxCHECK( !aSheetPath.Last() || ( aSheetPath.Last()->m_Uuid != m_Uuid ), /* void */ ); - - KIID_PATH path = aSheetPath.Path(); - for( SCH_SHEET_INSTANCE& instance : m_instances ) { - if( instance.m_Path == path ) + if( instance.m_Path == aPath ) { instance.m_PageNumber = aPageNumber; break; diff --git a/eeschema/sch_sheet.h b/eeschema/sch_sheet.h index 70ed16a2de..ff099a9cdb 100644 --- a/eeschema/sch_sheet.h +++ b/eeschema/sch_sheet.h @@ -474,40 +474,34 @@ protected: * in the list, do nothing. Sheet instances allow for the sharing in complex hierarchies * which allows for per instance data such as page number for sheets to stored. * - * @warning The #SCH_SHEET_PATH object must be a full hierarchical path which means the - * #SCH_SHEET object at index 0 must be the root sheet. A partial sheet path - * will raise an assertion on debug builds and silently fail and return false - * on release builds. + * @warning The #KIID_PATH object must be a full hierarchical path which means the sheet + * at index 0 must be the root sheet. * * @param[in] aInstance is the #SCH_SHEET_PATH of the sheet instance to the instance list. * @return false if the instance already exists, true if the instance was added. */ - bool addInstance( const SCH_SHEET_PATH& aInstance ); + bool addInstance( const KIID_PATH& aInstance ); /** * Return the sheet page number for \a aInstance. * - * @warning The #SCH_SHEET_PATH object must be a full hierarchical path which means the - * #SCH_SHEET object at index 0 must be the root sheet. A partial sheet path - * will raise an assertion on debug builds and silently fail and return an empty - * page number on release builds. + * @warning The #KIID_PATH object must be a full hierarchical path which means the sheet + * at index 0 must be the root sheet. * * @return the page number for the requested sheet instance. */ - wxString getPageNumber( const SCH_SHEET_PATH& aInstance ) const; + wxString getPageNumber( const KIID_PATH& aInstance ) const; /** * Set the page number for the sheet instance \a aInstance. * - * @warning The #SCH_SHEET_PATH object must be a full hierarchical path which means the - * #SCH_SHEET object at index 0 must be the root sheet. A partial sheet path - * will raise an assertion on debug builds and silently fail and return on release - * builds. + * @warning The #KIID_PATH object must be a full hierarchical path which means the sheet + * at index 0 must be the root sheet. * * @param[in] aInstance is the hierarchical path of the sheet. * @param[in] aReference is the new page number for the sheet. */ - void setPageNumber( const SCH_SHEET_PATH& aInstance, const wxString& aPageNumber ); + void setPageNumber( const KIID_PATH& aInstance, const wxString& aPageNumber ); bool getInstance( SCH_SHEET_INSTANCE& aInstance, const KIID_PATH& aSheetPath, bool aTestFromEnd = false ) const; diff --git a/eeschema/sch_sheet_path.cpp b/eeschema/sch_sheet_path.cpp index f04e9a4f8e..ac575f73dd 100644 --- a/eeschema/sch_sheet_path.cpp +++ b/eeschema/sch_sheet_path.cpp @@ -138,9 +138,10 @@ SCH_SHEET_PATH SCH_SHEET_PATH::operator+( const SCH_SHEET_PATH& aOther ) void SCH_SHEET_PATH::initFromOther( const SCH_SHEET_PATH& aOther ) { - m_sheets = aOther.m_sheets; - m_virtualPageNumber = aOther.m_virtualPageNumber; - m_current_hash = aOther.m_current_hash; + m_sheets = aOther.m_sheets; + m_virtualPageNumber = aOther.m_virtualPageNumber; + m_current_hash = aOther.m_current_hash; + m_cached_page_number = aOther.m_cached_page_number; // Note: don't copy m_recursion_test_cache as it is slow and we want std::vector to be // very fast to construct for use in the connectivity algorithm. @@ -327,6 +328,7 @@ wxString SCH_SHEET_PATH::PathAsString() const KIID_PATH SCH_SHEET_PATH::Path() const { KIID_PATH path; + path.reserve( m_sheets.size() ); for( const SCH_SHEET* sheet : m_sheets ) path.push_back( sheet->m_Uuid ); @@ -574,8 +576,7 @@ wxString SCH_SHEET_PATH::GetPageNumber() const wxCHECK( sheet, wxEmptyString ); - SCH_SHEET_PATH tmpPath = *this; - + KIID_PATH tmpPath = Path(); tmpPath.pop_back(); return sheet->getPageNumber( tmpPath ); @@ -588,7 +589,7 @@ void SCH_SHEET_PATH::SetPageNumber( const wxString& aPageNumber ) wxCHECK( sheet, /* void */ ); - SCH_SHEET_PATH tmpPath = *this; + KIID_PATH tmpPath = Path(); tmpPath.pop_back(); @@ -776,17 +777,27 @@ void SCH_SHEET_LIST::BuildSheetList( SCH_SHEET* aSheet, bool aCheckIntegrity ) void SCH_SHEET_LIST::SortByPageNumbers( bool aUpdateVirtualPageNums ) { + for( const SCH_SHEET_PATH& path : *this ) + path.CachePageNumber(); + std::sort( begin(), end(), - []( SCH_SHEET_PATH a, SCH_SHEET_PATH b ) -> bool + []( const SCH_SHEET_PATH& a, const SCH_SHEET_PATH& b ) -> bool { - int retval = a.ComparePageNum( b ); + int retval = SCH_SHEET::ComparePageNum( a.GetCachedPageNumber(), + b.GetCachedPageNumber() ); if( retval < 0 ) return true; else if( retval > 0 ) return false; - else /// Enforce strict ordering. If the page numbers are the same, use UUIDs - return a.GetCurrentHash() < b.GetCurrentHash(); + + if( a.GetVirtualPageNumber() < b.GetVirtualPageNumber() ) + return true; + else if( a.GetVirtualPageNumber() > b.GetVirtualPageNumber() ) + return false; + + /// Enforce strict ordering. If the page numbers are the same, use UUIDs + return a.GetCurrentHash() < b.GetCurrentHash(); } ); if( aUpdateVirtualPageNums ) @@ -794,9 +805,7 @@ void SCH_SHEET_LIST::SortByPageNumbers( bool aUpdateVirtualPageNums ) int virtualPageNum = 1; for( SCH_SHEET_PATH& sheet : *this ) - { sheet.SetVirtualPageNumber( virtualPageNum++ ); - } } } @@ -1321,7 +1330,7 @@ void SCH_SHEET_LIST::AddNewSheetInstances( const SCH_SHEET_PATH& aPrefixSheetPat for( SCH_SHEET_PATH& sheetPath : *this ) { - SCH_SHEET_PATH tmp( sheetPath ); + KIID_PATH tmp = sheetPath.Path(); SCH_SHEET_PATH newSheetPath( aPrefixSheetPath ); // Prefix the new hierarchical path. @@ -1339,10 +1348,10 @@ void SCH_SHEET_LIST::AddNewSheetInstances( const SCH_SHEET_PATH& aPrefixSheetPat SCH_SHEET_INSTANCE instance; // Add the instance if it doesn't already exist - if( !sheet->getInstance( instance, tmp.Path(), true ) ) + if( !sheet->getInstance( instance, tmp, true ) ) { sheet->addInstance( tmp ); - sheet->getInstance( instance, tmp.Path(), true ); + sheet->getInstance( instance, tmp, true ); } // Get a new page number if we don't have one diff --git a/eeschema/sch_sheet_path.h b/eeschema/sch_sheet_path.h index a95ab9a4bf..9b8736b160 100644 --- a/eeschema/sch_sheet_path.h +++ b/eeschema/sch_sheet_path.h @@ -229,6 +229,9 @@ public: */ int Cmp( const SCH_SHEET_PATH& aSheetPathToTest ) const; + void CachePageNumber() const { m_cached_page_number = GetPageNumber(); } + wxString GetCachedPageNumber() const { return m_cached_page_number; } + /** * Compare sheets by their page number. If the actual page number is equal, use virtual page * numbers to compare. @@ -416,9 +419,10 @@ private: void initFromOther( const SCH_SHEET_PATH& aOther ); protected: - std::vector< SCH_SHEET* > m_sheets; + std::vector m_sheets; - size_t m_current_hash; + size_t m_current_hash; + mutable wxString m_cached_page_number; int m_virtualPageNumber; /// Page numbers are maintained by the sheet load order. diff --git a/eeschema/tools/sch_find_replace_tool.cpp b/eeschema/tools/sch_find_replace_tool.cpp index 16c4b0fc46..d4d1bfe149 100644 --- a/eeschema/tools/sch_find_replace_tool.cpp +++ b/eeschema/tools/sch_find_replace_tool.cpp @@ -253,50 +253,39 @@ int SCH_FIND_REPLACE_TOOL::FindNext( const TOOL_EVENT& aEvent ) if( !item && searchAllSheets ) { - SCH_SCREENS screens( m_frame->Schematic().Root() ); - std::vector paths; + SCH_SCREENS screens( m_frame->Schematic().Root() ); + SCH_SHEET_LIST paths; screens.BuildClientSheetPathList(); for( SCH_SCREEN* screen = screens.GetFirst(); screen; screen = screens.GetNext() ) { for( SCH_SHEET_PATH& sheet : screen->GetClientSheetPaths() ) - paths.push_back( &sheet ); + paths.push_back( sheet ); } - std::sort( paths.begin(), paths.end(), [] ( const SCH_SHEET_PATH* lhs, - const SCH_SHEET_PATH* rhs ) -> bool - { - int retval = lhs->ComparePageNum( *rhs ); - - if( retval < 0 ) - return true; - else if( retval > 0 ) - return false; - else /// Enforce strict ordering. If the page numbers are the same, use UUIDs - return lhs->GetCurrentHash() < rhs->GetCurrentHash(); - } ); + paths.SortByPageNumbers( false ); if( isReversed ) std::reverse( paths.begin(), paths.end() ); - for( SCH_SHEET_PATH* sheet : paths ) + for( SCH_SHEET_PATH& sheet : paths ) { if( afterSheet ) { - if( afterSheet->GetCurrentHash() == sheet->GetCurrentHash() ) + if( afterSheet->GetCurrentHash() == sheet.GetCurrentHash() ) afterSheet = nullptr; continue; } - item = nextMatch( sheet->LastScreen(), sheet, nullptr, data, isReversed ); + item = nextMatch( sheet.LastScreen(), &sheet, nullptr, data, isReversed ); if( item ) { - if( m_frame->Schematic().CurrentSheet() != *sheet ) + if( m_frame->Schematic().CurrentSheet() != sheet ) { - m_frame->Schematic().SetCurrentSheet( *sheet ); + m_frame->Schematic().SetCurrentSheet( sheet ); m_frame->DisplayCurrentSheet(); } @@ -442,7 +431,7 @@ int SCH_FIND_REPLACE_TOOL::ReplaceAll( const TOOL_EVENT& aEvent ) } else { - SCH_SHEET_LIST allSheets = m_frame->Schematic().GetSheets(); + SCH_SHEET_LIST allSheets = m_frame->Schematic().GetUnorderedSheets(); SCH_SCREENS screens( m_frame->Schematic().Root() ); for( SCH_SCREEN* screen = screens.GetFirst(); screen; screen = screens.GetNext() )