kicad-developers team mailing list archive

[Maciej Sumiński <maciej.suminski@xxxxxxx>]

Jeff,

Thank you for the patches, I have just tested the one for KiCad. While I
do not see any difference when launching the footprint list dialog for
the first time (~6s on a debug build), the subsequent ones are
significantly faster (~2.6s reduced to ~0.6s).

I tested:
- removing a footprint from a library
- adding a footprint to a library
- disabling a library
- renaming a library
- removing a library
- adding a library

All changes were correctly recognized by the patched code.

For some reason I could not apply the patch with git am, but the
oldschool patch tool did not see any problems. I attach a version that
should apply cleanly with a few code formatting fixes and a safety
improvement (access to DEFAULT_COL_WIDTHS wrapped in a function).

Cheers,
Orson

On 02/09/2018 01:35 AM, Jeff Young wrote:
> Ping.
> 
> Any thoughts on patching wxWidgets for other platforms?
> 
> On 6 Feb 2018, at 14:08, Jeff Young <jeff@xxxxxxxxx<mailto:jeff@xxxxxxxxx>> wrote:
> 
> I accidentally dropped the dev list off the last few replies.  They included an update which fixed the issue Seth hypothesised.
> 
> But I’m updating it one more time because, well, I made it faster again.
> 
> There are two patches this time: the updated Kicad patch which knocks yet another 1/4 second off both first- and subsequent-times, and a wxWidgets patch which when combined with the Kicad patch brings the subsequent-time to near-instantaneous.
> 
> Since we maintain our own Mac wxWidgets there’s no reason not to get the full benefit there.  I’ll leave it up to others to decide whether to include the wxWidgets patch for other platforms.
> 
> (Note: while the two patches are dependent on each other for the final performance gain, they are not dependent on each other for compiling/linking/running.)
> 
> Cheers,
> Jeff.
> 
> 
> 
> 
> On 5 Feb 2018, at 22:48, Jeff Young <jeff@xxxxxxxxx<mailto:jeff@xxxxxxxxx>> wrote:
> 
> I think I must be having a thick moment, because I’m still not following you.
> 
> If I create a new library in the Footprint Editor and save it to a new location, then those components won’t be immediately available in Place Footprint until the library location is added to the Footprint Library Table, right?
> 
> Or am I completely missing something?
> 
> Cheers,
> Jeff.
> 
> On 5 Feb 2018, at 22:17, Wayne Stambaugh <stambaughw@xxxxxxxxx<mailto:stambaughw@xxxxxxxxx>> wrote:
> 
> On 02/05/2018 05:08 PM, Jeff Young wrote:
> Hi Wayne,
> Do you mean if I use a text editor to modify a module file while KiCad is running, will KiCad notice and reload it?  The answer is yes.  The checksum is generated as a hash of the disk directory last-modified dates; the lib-table only tells it what the current libraries are.
> 
> A text editor is one use case but the footprint library editor can also save an entire library file without going through the fp-lib-table so it's important that we don't break this behavior.  I may have missed this as I am sitting in Montreal airport after missing my connection with no sleep, a nasty case of jet lag, and an inbox that is sprialing out of control so I may have just overlooked it.
> 
> Nice talk at FOSDEM, by the way.  Do we all get t-shirts? ;)
> 
> Maybe one of these days I'll get around to some KiCad apparel of some type to give out to the dev team.  Although I don't think you should depend on my lack of graphics skills to design anything. :)
> 
> Cheers,
> 
> Wayne
> 
> 
> Cheers,
> Jeff.
> PS: updated patch attached to fix the issue Seth discovered (hypothesised?).
> On 5 Feb 2018, at 21:59, Wayne Stambaugh <stambaughw@xxxxxxxxx<mailto:stambaughw@xxxxxxxxx>> wrote:
> 
> What happens when footprint library file is modified outside the fp-lib-table?  At one point you could change the footprint library file without performing the file write through the fp-lib-table and the next time you accessed the library, it would recognize the file was modified and reload the cache.  Please make sure this behavior is not broken.
> 
> On 02/05/2018 04:51 PM, Jeff Young wrote:
> wxWidgets should return Now() which will make the checksums not match and trigger a reload.
> Of course what actually happens is that wxWidgets asserts. ;)
> New patch on the way….
> Cheers,
> Jeff.
> On 5 Feb 2018, at 21:42, Seth Hillbrand <seth.hillbrand@xxxxxxxxx<mailto:seth.hillbrand@xxxxxxxxx> <mailto:seth.hillbrand@xxxxxxxxx>> wrote:
> 
> Jeff-
> 
> Will removing a footprint library trigger a refresh?  Or will that mess up the checksum calculation?
> 
> -S
> 
> 2018-02-05 13:04 GMT-08:00 Jeff Young <jeff@xxxxxxxxx<mailto:jeff@xxxxxxxxx> <mailto:jeff@xxxxxxxxx>>:
> 
>   This one for Place Footprint (specifically the List All dialog
>   found therein).
> 
>   On my machine it knocks the first-use time from 4s to 3s and the
>   subsequent-use time from 2.5s to 1s.
> 
>   Cheers,
>   Jeff.
> 
> 
>   _______________________________________________
>   Mailing list: https://launchpad.net/~kicad-developers
>   <https://launchpad.net/~kicad-developers>
>   Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx<mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>   <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
>   Unsubscribe : https://launchpad.net/~kicad-developers
>   <https://launchpad.net/~kicad-developers>
>   More help   : https://help.launchpad.net/ListHelp
>   <https://help.launchpad.net/ListHelp>
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx<mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx> <mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx<mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx<mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 
> 
> _______________________________________________
> Mailing list: https://launchpad.net/~kicad-developers
> Post to     : kicad-developers@xxxxxxxxxxxxxxxxxxx<mailto:kicad-developers@xxxxxxxxxxxxxxxxxxx>
> Unsubscribe : https://launchpad.net/~kicad-developers
> More help   : https://help.launchpad.net/ListHelp
> 

From d61905ea358e57e865de2d512ee69544b30fc201 Mon Sep 17 00:00:00 2001
From: Jeff Young <jeff@xxxxxxxxx>
Date: Fri, 9 Feb 2018 09:04:04 +0100
Subject: [PATCH] Performance fixes for the place footprint list all dialog.

Smartens the cache freshness checking, and adds checksums of
constituent-directory last-mod-dates to the footprint info
list.

Also inserts the dataPtrs into the list at the same time as
the text to keep wxWidgets from measuring the width of the
text twice (yes, really).  And converts the list to default-
column widths in case the wxWidgets patch is present to
greatly speed list creation (by not measuring all that text).

On my machine drops the first-load time from 4s to 2.5s and
the subsequent-load times from 2.5s to < 1s.  With the
wxWidgets patch subsequent-loads become near-instantaneous.
---
 common/displlst.cpp            | 52 +++++++++++++++++++++------------------
 common/fp_lib_table.cpp        | 22 +++++++++++++++++
 include/fp_lib_table.h         |  6 +++++
 pcbnew/footprint_info_impl.cpp |  9 ++++++-
 pcbnew/footprint_info_impl.h   |  1 +
 pcbnew/io_mgr.h                | 10 ++++++++
 pcbnew/kicad_plugin.cpp        | 55 +++++++++++-------------------------------
 pcbnew/kicad_plugin.h          |  4 ++-
 8 files changed, 92 insertions(+), 67 deletions(-)

diff --git a/common/displlst.cpp b/common/displlst.cpp
index 7e2eddda2..75a885b15 100644
--- a/common/displlst.cpp
+++ b/common/displlst.cpp
@@ -33,6 +33,20 @@
 #include <kicad_string.h>
 #include <dialog_helpers.h>
 
+
+static inline int getDefaultColWidth( int aIdx )
+{
+    // wxWidgets spends *far* too long calcuating column widths (most of it, believe it or
+    // not, in repeatedly creating/destorying a wxDC to do the measurement in).
+    // Use default column widths instead.  (Note that these will be scaled down proportionally
+    // to fit the available space when the dialog is instantiated.)
+    static const std::vector<int> DEFAULT_COL_WIDTHS = { 400, 200 };
+    wxASSERT( aIdx == DEFAULT_COL_WIDTHS.size() );
+
+    return aIdx < DEFAULT_COL_WIDTHS.size() ? DEFAULT_COL_WIDTHS[aIdx] : wxLIST_AUTOSIZE;
+}
+
+
 EDA_LIST_DIALOG::EDA_LIST_DIALOG( EDA_DRAW_FRAME* aParent, const wxString& aTitle,
                                   const wxArrayString& aItemHeaders,
                                   const std::vector<wxArrayString>& aItemList,
@@ -64,19 +78,11 @@ EDA_LIST_DIALOG::EDA_LIST_DIALOG( EDA_DRAW_FRAME* aParent, const wxString& aTitl
 }
 
 
-void EDA_LIST_DIALOG::initDialog( const wxArrayString& aItemHeaders,
-                                  const wxString& aSelection)
+void EDA_LIST_DIALOG::initDialog( const wxArrayString& aItemHeaders, const wxString& aSelection )
 {
-
     for( unsigned i = 0; i < aItemHeaders.Count(); i++ )
-    {
-        wxListItem column;
-
-        column.SetId( i );
-        column.SetText( aItemHeaders.Item( i ) );
-
-        m_listBox->InsertColumn( i, column );
-    }
+        m_listBox->InsertColumn( i, aItemHeaders.Item( i ),
+                wxLIST_FORMAT_LEFT, getDefaultColWidth( i ) );
 
     InsertItems( *m_itemsListCp, 0 );
 
@@ -86,15 +92,6 @@ void EDA_LIST_DIALOG::initDialog( const wxArrayString& aItemHeaders,
         m_staticTextMsg->Show( false );
     }
 
-    for( unsigned col = 0; col < aItemHeaders.Count();  ++col )
-    {
-        m_listBox->SetColumnWidth( col, wxLIST_AUTOSIZE );
-        int columnwidth = m_listBox->GetColumnWidth( col );
-        m_listBox->SetColumnWidth( col, wxLIST_AUTOSIZE_USEHEADER );
-        int headerwidth = m_listBox->GetColumnWidth( col );
-        m_listBox->SetColumnWidth( col, std::max( columnwidth, headerwidth ) );
-    }
-
     if( !!aSelection )
     {
         for( unsigned row = 0; row < m_itemsListCp->size(); ++row )
@@ -177,18 +174,25 @@ void EDA_LIST_DIALOG::InsertItems( const std::vector< wxArrayString >& itemList,
     {
         wxASSERT( (int) itemList[row].GetCount() == m_listBox->GetColumnCount() );
 
-        long itemIndex = 0;
         for( unsigned col = 0; col < itemList[row].GetCount(); col++ )
         {
+            wxListItem info;
+            info.m_itemId = row + position;
+            info.m_col = col;
+            info.m_text = itemList[row].Item( col );
+            info.m_width = getDefaultColWidth( col );
+            info.m_mask = wxLIST_MASK_TEXT | wxLIST_MASK_WIDTH;
 
             if( col == 0 )
             {
-                itemIndex = m_listBox->InsertItem( row+position, itemList[row].Item( col ) );
-                m_listBox->SetItemPtrData( itemIndex, wxUIntPtr( &itemList[row].Item( col ) ) );
+                info.m_data = wxUIntPtr( &itemList[row].Item( col ) );
+                info.m_mask |= wxLIST_MASK_DATA;
+
+                m_listBox->InsertItem( info );
             }
             else
             {
-                m_listBox->SetItem( itemIndex, col, itemList[row].Item( col ) );
+                m_listBox->SetItem( info );
             }
         }
     }
diff --git a/common/fp_lib_table.cpp b/common/fp_lib_table.cpp
index a51a1c94f..e83c326f9 100644
--- a/common/fp_lib_table.cpp
+++ b/common/fp_lib_table.cpp
@@ -235,6 +235,28 @@ void FP_LIB_TABLE::Format( OUTPUTFORMATTER* aOutput, int aIndentLevel ) const
 }
 
 
+long long FP_LIB_TABLE::GenLastModifiedChecksum( const wxString* aNickname )
+{
+    if( aNickname )
+    {
+        const FP_LIB_TABLE_ROW* row = FindRow( *aNickname );
+        wxASSERT( (PLUGIN*) row->plugin );
+        return row->plugin->GetLibModificationTime( *aNickname ).GetValue().GetValue();
+    }
+
+    long long hash = 0;
+
+    for( wxString const& nickname : GetLogicalLibs() )
+    {
+        const FP_LIB_TABLE_ROW* row = FindRow( nickname );
+        wxASSERT( (PLUGIN*) row->plugin );
+        hash += row->plugin->GetLibModificationTime( nickname ).GetValue().GetValue();
+    }
+
+    return hash;
+}
+
+
 void FP_LIB_TABLE::FootprintEnumerate( wxArrayString& aFootprintNames, const wxString& aNickname )
 {
     const FP_LIB_TABLE_ROW* row = FindRow( aNickname );
diff --git a/include/fp_lib_table.h b/include/fp_lib_table.h
index 64c561f20..ffa09d3fb 100644
--- a/include/fp_lib_table.h
+++ b/include/fp_lib_table.h
@@ -152,6 +152,12 @@ public:
     void FootprintEnumerate( wxArrayString& aFootprintNames, const wxString& aNickname );
 
     /**
+     * Generate a checksum of the last-mod-date of \a aNickname's directory, or a checksum
+     * of all the libraries' directories if \a aNickname is NULL.
+     */
+    long long GenLastModifiedChecksum( const wxString* aNickname );
+
+    /**
      * Function PrefetchLib
      * If possible, prefetches the specified library (e.g. performing downloads). Does not parse.
      * Threadsafe.
diff --git a/pcbnew/footprint_info_impl.cpp b/pcbnew/footprint_info_impl.cpp
index 56bf260cb..4afb7159f 100644
--- a/pcbnew/footprint_info_impl.cpp
+++ b/pcbnew/footprint_info_impl.cpp
@@ -123,11 +123,18 @@ void FOOTPRINT_LIST_IMPL::loader_job()
 
 bool FOOTPRINT_LIST_IMPL::ReadFootprintFiles( FP_LIB_TABLE* aTable, const wxString* aNickname )
 {
+    if( aTable->GenLastModifiedChecksum( aNickname ) == m_libraries_last_mod_checksum )
+        return true;
+
     FOOTPRINT_ASYNC_LOADER loader;
 
     loader.SetList( this );
     loader.Start( aTable, aNickname );
-    return loader.Join();
+    bool retval = loader.Join();
+
+    m_libraries_last_mod_checksum = aTable->GenLastModifiedChecksum( aNickname );
+
+    return retval;
 }
 
 
diff --git a/pcbnew/footprint_info_impl.h b/pcbnew/footprint_info_impl.h
index fd539655e..14c80acba 100644
--- a/pcbnew/footprint_info_impl.h
+++ b/pcbnew/footprint_info_impl.h
@@ -62,6 +62,7 @@ class FOOTPRINT_LIST_IMPL : public FOOTPRINT_LIST
     SYNC_QUEUE<wxString>     m_queue_out;
     std::atomic_bool         m_first_to_finish;
     std::atomic_size_t       m_count_finished;
+    long long                m_libraries_last_mod_checksum;
 
     /**
      * Call aFunc, pushing any IO_ERRORs and std::exceptions it throws onto m_errors.
diff --git a/pcbnew/io_mgr.h b/pcbnew/io_mgr.h
index ae829e95f..9435c4371 100644
--- a/pcbnew/io_mgr.h
+++ b/pcbnew/io_mgr.h
@@ -28,6 +28,7 @@
 #include <richio.h>
 #include <map>
 #include <functional>
+#include <wx/time.h>
 
 class BOARD;
 class PLUGIN;
@@ -353,6 +354,15 @@ public:
                                      const PROPERTIES* aProperties = NULL );
 
     /**
+     * Return the footprint lib last-mod-time, if available.
+     */
+    virtual wxDateTime GetLibModificationTime( const wxString& aLibraryPath ) const
+    {
+        // Default implementation.
+        return wxDateTime::Now();   // If we don't know then we must assume the worst.
+    }
+
+    /**
      * Function PrefetchLib
      * If possible, prefetches the specified library (e.g. performing downloads). Does not parse.
      * Threadsafe.
diff --git a/pcbnew/kicad_plugin.cpp b/pcbnew/kicad_plugin.cpp
index d4868db5b..dc92aab5f 100644
--- a/pcbnew/kicad_plugin.cpp
+++ b/pcbnew/kicad_plugin.cpp
@@ -200,6 +200,7 @@ public:
 
 
 FP_CACHE::FP_CACHE( PCB_IO* aOwner, const wxString& aLibraryPath )
+    : m_mod_time( 0.0 )
 {
     m_owner = aOwner;
     m_lib_path.SetPath( aLibraryPath );
@@ -208,6 +209,9 @@ FP_CACHE::FP_CACHE( PCB_IO* aOwner, const wxString& aLibraryPath )
 
 wxDateTime FP_CACHE::GetLibModificationTime() const
 {
+    if( !m_lib_path.DirExists() )
+        return wxDateTime::Now();
+
     return m_lib_path.GetModificationTime();
 }
 
@@ -368,47 +372,7 @@ bool FP_CACHE::IsPath( const wxString& aPath ) const
 
 bool FP_CACHE::IsModified( const wxString& aLibPath, const wxString& aFootprintName ) const
 {
-    // The library is modified if the library path got deleted or changed.
-    if( !m_lib_path.DirExists() || !IsPath( aLibPath ) )
-        return true;
-
-    // If no footprint was specified, check every file modification time against the time
-    // it was loaded.
-    if( aFootprintName.IsEmpty() )
-    {
-        for( MODULE_CITER it = m_modules.begin();  it != m_modules.end();  ++it )
-        {
-            wxFileName fn = m_lib_path;
-
-            fn.SetName( it->second->GetFileName().GetName() );
-            fn.SetExt( KiCadFootprintFileExtension );
-
-            if( !fn.FileExists() )
-            {
-                wxLogTrace( traceFootprintLibrary,
-                            wxT( "Footprint cache file '%s' does not exist." ),
-                            fn.GetFullPath().GetData() );
-                return true;
-            }
-
-            if( it->second->IsModified() )
-            {
-                wxLogTrace( traceFootprintLibrary,
-                            wxT( "Footprint cache file '%s' has been modified." ),
-                            fn.GetFullPath().GetData() );
-                return true;
-            }
-        }
-    }
-    else
-    {
-        MODULE_CITER it = m_modules.find( aFootprintName );
-
-        if( it == m_modules.end() || it->second->IsModified() )
-            return true;
-    }
-
-    return false;
+    return GetLibModificationTime() > m_mod_time;
 }
 
 
@@ -2140,6 +2104,15 @@ void PCB_IO::FootprintDelete( const wxString& aLibraryPath, const wxString& aFoo
 }
 
 
+wxDateTime PCB_IO::GetLibModificationTime( const wxString& aLibraryPath ) const
+{
+    if( !m_cache )
+        return wxDateTime::Now();     // force a load
+
+    return m_cache->GetLibModificationTime();
+}
+
+
 void PCB_IO::FootprintLibCreate( const wxString& aLibraryPath, const PROPERTIES* aProperties )
 {
     if( wxDir::Exists( aLibraryPath ) )
diff --git a/pcbnew/kicad_plugin.h b/pcbnew/kicad_plugin.h
index 0c0792a36..c5ae9a13a 100644
--- a/pcbnew/kicad_plugin.h
+++ b/pcbnew/kicad_plugin.h
@@ -43,7 +43,7 @@ class NETINFO_MAPPING;
 //                                              // went to 32 Cu layers from 16.
 //#define SEXPR_BOARD_FILE_VERSION    20160815  // differential pair settings per net class
 //#define SEXPR_BOARD_FILE_VERSION    20170123  // EDA_TEXT refactor, moved 'hide'
-//#define SEXPR_BOARD_FILE_VERSION    20170920  // long pad names and custom pad shape
+//#define SEXPR_BOARD_FILE_VERSION    20170920  // long pad names and custom pad shape
 //#define SEXPR_BOARD_FILE_VERSION    20170922  // Keepout zones can exist on multiple layers
 //#define SEXPR_BOARD_FILE_VERSION    20171114  // Save 3D model offset in mm, instead of inches
 //#define SEXPR_BOARD_FILE_VERSION    20171125  // Locked/unlocked TEXTE_MODULE
@@ -130,6 +130,8 @@ public:
     void FootprintDelete( const wxString& aLibraryPath, const wxString& aFootprintName,
                           const PROPERTIES* aProperties = NULL ) override;
 
+    wxDateTime GetLibModificationTime( const wxString& aLibraryPath ) const override;
+
     void FootprintLibCreate( const wxString& aLibraryPath, const PROPERTIES* aProperties = NULL) override;
 
     bool FootprintLibDelete( const wxString& aLibraryPath, const PROPERTIES* aProperties = NULL ) override;
-- 
2.13.3

Attachment: signature.asc
Description: OpenPGP digital signature