kicad-developers team mailing list archive

[Chris Pavlina <pavlina.chris@xxxxxxxxx>]

The RescueProject() optimization looks good to me.

On Thu, Mar 09, 2017 at 08:59:33AM +0100, jp charras wrote:
> Le 08/03/2017 à 22:50, Chris Pavlina a écrit :
> > On Wed, Mar 08, 2017 at 04:33:06PM -0500, Wayne Stambaugh wrote:
> >> On 3/8/2017 4:08 PM, Chris Pavlina wrote:
> >>> That's why I submitted such a trivial patch to the list first, I figured
> >>> someone would say something ;)
> >>>
> >>> It does make sense to me, because the GUI is blocked. The busy cursor
> >>> says to me "yes, the GUI is supposed to be blocked right now, it's not
> >>> frozen". Even with a progress bar, it can seem unresponsive -
> >>> particularly if 1) the progress bar ends up obscured, as can happen with
> >>> 'weird' window managers sometimes, or 2) if a single library takes a
> >>> particularly long time to load, which I'm sure will only get worse if we
> >>> eventually allow loading them over the internet like for footprint libs.
> >>
> >> It's the old belt and suspender method.  Users have got to quit using
> >> those 'weird' window managers.  It causes way too much grief.
> > 
> > I'm not sure displaying a busy cursor while the GUI is frozen is grief.
> > I've always considered it correct behavior, to be honest, regardless of
> > the presence or absence of a weird window manager. The busy cursor has
> > been used for _years_ to indicate a GUI that cannot be interacted with,
> > so users expect it - not displaying it sends conflicting messages; you
> > are saying by the absence of such a standard item that the GUI is ready
> > to use.
> > 
> >> I'm
> >> surprised that a single library load takes long enough to need a busy
> >> cursor but I'm not opposed to the patch.  Does the progress bar in
> >> wxWidgets have a continuous mode?  That doesn't solve the hidden
> >> progress window issue though.
> > 
> > There is a continuous mode, though I'm not sure what the benefit of
> > using it would be, and it's a bit strange (you have to "poke" it pretty
> > continuously or it freezes). I wouldn't use it.
> > 
> > I'm not sure why library loading got so slow suddenly. I'd profile it
> > but I don't have time.
> 
> I had a look at this.
> I am not sure the library loading itself is really slower.
> 
> the loading time is mainly due to the call to SCH_EDIT_FRAME::RescueProject().
> (previously: 0.5 seconds, now 14 seconds with my large schematic test, but with not a lot of libraries)
> 
> I am thinking the root problem is SCH_COMPONENT::Resolve(), now much more time consuming.
> 
> One other time consuming is SCH_COMPONENT::ResolveAll (previously: a few ms, now 1,4 s with this
> schematic) due to the same reason.
> 
> Both methods are not optimized, and the call to Resolve() is made many times for the same lib id.
> 
> I optimized SCH_COMPONENT::ResolveAll to call Resolve() only once by lib id (see the attached patch.
> (previously: 1.4 s and 0.14 s with fast_sch_component-ResolveAll attached patch)
> 
> I don't know why Resolve() is now so time consuming.
> (A fast search algo could be interesting, but I don't think the previous method was optimized)
> 
> SCH_EDIT_FRAME::RescueProject() could be also optimized to minimize the calculations: there are a
> lot of redundant calculations.
> This is the more time consuming, due to intensive and not optimized use of SCH_COMPONENT::Resolve().
> 
> As a proof, I attached a basic patch (fast_sch_project_rescue_stub) with optimized search.
> However this patch is only a proof, and cannot be used as this, because it searches for modified
> symbols, but does not modify all corresponding components in schematic, only one (but the missing
> feature is not time consuming).
> The calculation time was 13 s and 0.5s with this patch.
> (You also can disable the call to RescueProject() in Eeschema: this is an option)
> 
> 
> I can commit fast_sch_component-ResolveAll patch (If Wayne agrees) because it works fine for me.
> The other patch needs more work.
> 
> -- 
> Jean-Pierre CHARRAS