kicad-developers team mailing list archive

[Jon Evans <jon@xxxxxxxxxxxxx>]

Hi Orson,

I have dug into Seth's report of slowness on this large Gerber file, and
reproduced it.
It looks like VIEW is struggling under the load of all the items, so this
is a generic GAL problem, but Gerber files in general have many more draw
items than PCB files, so we only notice it now.

I have played around a bit with some profiling and optimizations, but have
gotten stuck on one.
It seems like RTREE::Remove is very costly, and so one optimization I tried
is to skip the remove/add step in VIEW::updateLayers() if the layers are
the same.
This has some subtle problems that I don't understand, so maybe you can
advise?

1) If I compile the code with the attached patch, GerbView is much faster
(1 minute load time -> 5 seconds on Seth's file), but the message box info
doesn't show up for selected items.  In PcbNew, selected items are
invisible.
2) If I comment out line 1262 ("if( !same )") to unconditionally run the
insert, the message box and invisible items problems are solved, but the
programs segfault after selecting a few items.

I don't have a good understanding of the inner workings of the R-Tree or
whether Remove could be optimized so that we wouldn't have to skip it, but
I also don't understand why if updateLayers() is called and the layers of
an item haven't changed, it seems like we still have to do the remove and
insert loops to get proper behavior.


Thanks,
-Jon

On Thu, Sep 21, 2017 at 3:13 PM, Seth Hillbrand <seth.hillbrand@xxxxxxxxx>
wrote:

> Hi Jon-
>
> Sorry, my wording was bad.  This is not a merging show-stopper.  Legacy
> use is unaffected.
>
> Best-
> Seth
>
> On Thu, Sep 21, 2017 at 11:55 AM, Jon Evans <jon@xxxxxxxxxxxxx> wrote:
>
>> Hi Seth,
>>
>> Thanks for reporting and providing your example file.  I will investigate
>> this issue, but is this actually a show stopper for merging?  Can't you
>> just keep using legacy canvas?
>>
>> Thanks,
>> Jon
>>
>> On Thu, Sep 21, 2017 at 2:47 PM, Seth Hillbrand <seth.hillbrand@xxxxxxxxx
>> > wrote:
>>
>>> -- Apologies if this message is a repeat.  Looks like first attempt
>>> didn't go through --
>>>
>>> Hi Jon-
>>>
>>> This is really neat work!  I especially like the selection highlighting.
>>>
>>> However, I do have a show-stopping (for me) issue.  Loading a few of my
>>> projects takes a really long time.  Using the legacy canvas, I can load the
>>> attached layer in about 2 seconds on my relatively modest machine.  When I
>>> use the OpenGL canvas, it takes almost a full minute.  Same again when I
>>> zoom and scroll.  There is no lag on the legacy canvas, but at least 2-3
>>> seconds to zoom or scroll under OpenGL.
>>>
>>> I am running Linux, latest NVidia drivers on a GeForce 750 GTX.
>>>
>>> Best-
>>> Seth
>>>
>>> On Thu, Sep 21, 2017 at 3:05 AM, Maciej Sumiński  wrote:
>>>
>>>> Hi Jon,
>>>>
>>>> Thanks you, this is really cool! Now it is even more tempting to merge
>>>> the gerbview_gal branch. I am going to wait one more day for vetos and
>>>> tomorrow I will push it to the master branch.
>>>>
>>>> Regards,
>>>> Orson
>>>>
>>>> On 09/20/2017 09:57 PM, Jon Evans wrote:
>>>> > Hi Orson,
>>>> >
>>>> > Give this a shot in your branch.  It should work in pcbnew also now.
>>>> >
>>>> > -Jon
>>>> >
>>>> > On Wed, Sep 20, 2017 at 9:28 AM, Jon Evans <jon@xxxxxxxxxxxxx> wrote:
>>>> >
>>>> >> Hi Orson,
>>>> >>
>>>> >> Thank you for staging this for merge on your branch.  I checked and
>>>> you do
>>>> >> have all the patches.
>>>> >>
>>>> >> 1) Yes I planned on refactoring the selection tool once things
>>>> stabilized
>>>> >> with the highlighting etc.
>>>> >> 2) Do you mean when you are highlighting Gerber X2 attributes, or
>>>> when you
>>>> >> are deselecting things, or something else?
>>>> >> 3) That's a good idea on VIEW_GROUP, I will give it a try and send a
>>>> patch.
>>>> >>
>>>> >> Thanks,
>>>> >> Jon
>>>> >>
>>>> >> On Wed, Sep 20, 2017 at 5:46 AM, Maciej Sumiński <
>>>> maciej.suminski@xxxxxxx>
>>>> >> wrote:
>>>> >>
>>>> >>> Hi Jon,
>>>> >>>
>>>> >>> GALifying GerbView is a huge task, so thank you very much for your
>>>> work!
>>>> >>> I have just tested your changes and in my opinion it is in a state
>>>> that
>>>> >>> deserves merging and further tests. The new way of item
>>>> highlighting is
>>>> >>> awesome, we need to port it to pcbnew as well.
>>>> >>>
>>>> >>> For now I keep your patches in a separate branch, with some minor
>>>> >>> modifications on top of it [1]. Please verify it contains all the
>>>> needed
>>>> >>> patches. If nobody objects, I would like to merge it this week.
>>>> >>>
>>>> >>> Just a few minor remarks:
>>>> >>> - It seems there is some code that could be refactored to share it
>>>> with
>>>> >>> pcbnew (e.g. selection tool).
>>>> >>> - 'Clear highlight' operation takes long time to finish (seems more
>>>> than
>>>> >>> with the legacy canvas), but I cannot really see what is happening
>>>> >>> there. If it cannot be easily fixed, perhaps it could set the mouse
>>>> >>> cursor to busy.
>>>> >>> - For the new highlighting method: perhaps a more universal way is
>>>> to
>>>> >>> create a temporary VIEW_GROUP object containing the selection
>>>> candidate.
>>>> >>> This way it can be temporarily displayed on the overlay layer,
>>>> without
>>>> >>> modifying the original ViewGetLayer() methods.
>>>> >>>
>>>> >>> Regards,
>>>> >>> Orson
>>>> >>>
>>>> >>> 1. https://code.launchpad.net/~orsonmmz/kicad/+git/kicad/+ref/
>>>> >>> gerbview_gal
>>>> >>>
>>>> >>> On 09/18/2017 12:47 AM, Jon Evans wrote:
>>>> >>>> Hi all,
>>>> >>>>
>>>> >>>> The day has finally come!  I have distilled my GerbView GAL branch
>>>> into
>>>> >>> a
>>>> >>>> patchset attached to this email.  Hopefully with this merged into
>>>> >>> master we
>>>> >>>> can identify any remaining bugs and clean it up for 5.0.
>>>> >>>>
>>>> >>>> Note that this set is split into 5 patches to make review easier,
>>>> but
>>>> >>> they
>>>> >>>> are not intended to compile and work independently.
>>>> >>>>
>>>> >>>> Best,
>>>> >>>> Jon
>>>> >>>>
>>>> >>>>
>>>> >>>>
>>>> >>>> _______________________________________________
>>>> >>>> Mailing list: https://launchpad.net/~kicad-developers
>>>> >>>> Post to     : 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
>>>> >>> 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
>>>> 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
>>> Unsubscribe : https://launchpad.net/~kicad-developers
>>> More help   : https://help.launchpad.net/ListHelp
>>>
>>>
>>
>

From b9d89465299736663d1c84903e2f56b1c3c0091a Mon Sep 17 00:00:00 2001
From: Jon Evans <jon@xxxxxxxxxxxxx>
Date: Thu, 21 Sep 2017 21:56:31 -0400
Subject: [PATCH] Buggy attempt to optimize VIEW::updateLayers()

---
 common/view/view.cpp | 58 +++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 10 deletions(-)

diff --git a/common/view/view.cpp b/common/view/view.cpp
index 80536ce1a..bf49a18cd 100644
--- a/common/view/view.cpp
+++ b/common/view/view.cpp
@@ -78,13 +78,22 @@ private:
     {
         int* layersPtr = aLayers;
 
+        aCount = m_layers.count();
+
+        // Return from the cache if we have up to four layers
+        if( aCount < 4 )
+        {
+            for( int i = 0; i < aCount; ++i )
+                *layersPtr++ = m_layersCache[i];
+
+            return;
+        }
+
         for( unsigned int i = 0; i < m_layers.size(); ++i )
         {
             if( m_layers[i] )
                 *layersPtr++ = i;
         }
-
-        aCount = m_layers.count();
     }
 
     VIEW*   m_view;             ///< Current dynamic view the item is assigned to.
@@ -195,6 +204,9 @@ private:
     /// Stores layer numbers used by the item.
     std::bitset<VIEW::VIEW_MAX_LAYERS> m_layers;
 
+    /// Optimization: cache the first four layers
+    int m_layersCache[4];
+
     /**
      * Function saveLayers()
      * Saves layers used by the item.
@@ -212,6 +224,9 @@ private:
             wxASSERT( unsigned( aLayers[i] ) <= unsigned( VIEW::VIEW_MAX_LAYERS ) );
 
             m_layers.set( aLayers[i] );
+
+            if( i < 4 )
+                m_layersCache[i] = aLayers[i];
         }
     }
 
@@ -1194,6 +1209,7 @@ void VIEW::updateLayers( VIEW_ITEM* aItem )
 {
     auto viewData = aItem->viewPrivData();
     int layers[VIEW_MAX_LAYERS], layers_count;
+    int new_layers[VIEW_MAX_LAYERS], new_layers_count;
 
     if( !viewData )
         return;
@@ -1201,11 +1217,31 @@ void VIEW::updateLayers( VIEW_ITEM* aItem )
     // Remove the item from previous layer set
     viewData->getLayers( layers, layers_count );
 
+    // RTREE::Remove is expensive, so first let's check if it's needed
+    aItem->ViewGetLayers( new_layers, new_layers_count );
+
+    bool same = ( layers_count == new_layers_count );
+
+    if (same)
+    {
+        for( int i = 0; i < layers_count; ++i )
+        {
+            if( same && ( layers[i] != new_layers[i] ) )
+            {
+                same = false;
+            }
+        }
+    }
+
     for( int i = 0; i < layers_count; ++i )
     {
         VIEW_LAYER& l = m_layers[layers[i]];
-        l.items->Remove( aItem );
-        MarkTargetDirty( l.target );
+
+        if( !same )
+        {
+            l.items->Remove( aItem );
+            MarkTargetDirty( l.target );
+        }
 
         if( IsCached( l.id ) )
         {
@@ -1221,14 +1257,16 @@ void VIEW::updateLayers( VIEW_ITEM* aItem )
     }
 
     // Add the item to new layer set
-    aItem->ViewGetLayers( layers, layers_count );
-    viewData->saveLayers( layers, layers_count );
+    viewData->saveLayers( new_layers, new_layers_count );
 
-    for( int i = 0; i < layers_count; i++ )
+    if( !same )
     {
-        VIEW_LAYER& l = m_layers[layers[i]];
-        l.items->Insert( aItem );
-        MarkTargetDirty( l.target );
+        for( int i = 0; i < new_layers_count; i++ )
+        {
+            VIEW_LAYER& l = m_layers[new_layers[i]];
+            l.items->Insert( aItem );
+            MarkTargetDirty( l.target );
+        }
     }
 }
 
-- 
2.11.0