[chirp_devel] [PATCH] [csv] Optimize generic csv files load times. Fixes #8991
Kosta Arvanitis
Sun Jun 13 17:11:03 PDT 2021
Thanks for taking the time to review the change Dan. I have posted my
comments inline below, I will attempt to turn around a commit quickly so as
not to take up more of your time.
-kosta
On Fri, Jun 11, 2021 at 4:59 PM Dan Smith via chirp_devel <
chirp_devel at intrepid.danplanet.com> wrote:
> > # HG changeset patch
> > # User Kosta A. <ve7kcy at gmail.com>
> > # Date 1618643404 25200
> > # Sat Apr 17 00:10:04 2021 -0700
> > # Node ID cd3e2444040876b4a19b41c6cfecedb79ff4a8fe
> > # Parent 42c5ae551170c33b04f997e3a07a9e1857bdb504
> > [csv] Optimize generic csv files load times. Fixes #8991
>
> This patch does this, but a lot more. I'd like some more words here about
> what it does for speedups, and splitting out the other parts of the change.
> If you can turn this around quickly, I'll commit to getting it in this
> weekend, so you don't have to wait again. If you can't, let me know and
> I'll do it for you.
>
> > - def __init__(self):
> > + def __init__(self, number=0, empty=False, name=""):
> > self.freq = 0
> > - self.number = 0
> > + self.number = number
> > self.extd_number = ""
> > - self.name = ""
> > + self.name = name
> > self.vfo = 0
> > self.rtone = 88.5
> > self.ctone = 88.5
> > @@ -290,7 +290,7 @@
> >
> > self.comment = ""
> >
> > - self.empty = False
> > + self.empty = empty
>
> This is fine, but how about we just go with something like this to make it
> even more flexible?
>
> def __init__(self, **attrs):
> self.freq = 0
> self.number = 0
> self.extd_number = ""
> # ... etc
> for k, v in attrs.items():
> setattr(self, k, v)
>
> Then you can initialize any of the fields in Memory when you create it,
> which will be nicer all around.
>
> This change was specifically made as an optimization as memories are
constructed in a rather tight loop; it was slightly faster to remove the
double member assignment and replace it with a parametrized constructor.
While a generic iterator would be flexible, I was not aiming for added
flexibility here as much as reducing the overall number of instructions for
the add perf.
>
> > @directory.register
> > -class CSVRadio(chirp_common.FileBackedRadio,
> chirp_common.IcomDstarSupport):
> > +class CSVRadio(chirp_common.FileBackedRadio):
> > """A driver for Generic CSV files"""
> > VENDOR = "Generic"
> > MODEL = "CSV"
> > @@ -67,20 +67,15 @@
> > "Mode": (str, "mode"),
> > "TStep": (float, "tuning_step"),
> > "Skip": (str, "skip"),
> > - "URCALL": (str, "dv_urcall"),
> > - "RPT1CALL": (str, "dv_rpt1call"),
> > - "RPT2CALL": (str, "dv_rpt2call"),
> > "Comment": (str, "comment"),
> > }
>
> This is a pretty major change that has nothing to do with the speed
> improvements, removes functionality, and isn't even mentioned in the commit
> message. I don't really mind dropping the DstarSupport from this because I
> don't think it's useful for anything other than the first generation
> radios, and wasn't really that useful then anyway. But, let's please make
> it a separate commit so that it stands alone.
>
> You are correct about this change, I can separate it out np.
> > - def _blank(self):
> > + def _blank(self, defaults=False):
> > self.errors = []
> > - self.memories = []
> > - for i in range(0, 1000):
> > - mem = chirp_common.Memory()
> > - mem.number = i
> > - mem.empty = True
> > - self.memories.append(mem)
> > + self.memories = [chirp_common.Memory(i, True) for i in range(0,
> 1000)]
> > + if (defaults):
> > + self.memories[0].empty = False
> > + self.memories[0].freq = 146010000
>
> I expect this listcomp is the bulk of the intended performance improvement
> here, right?
>
> To be honest, I can't really tell a difference in the load times on my
> machine. Does this patch make a noticeable improvement for you (and others)?
>
> The 'bulk' of the realized optimization stems from the removal of the
redundant call to prefill - which is best visualized without a profiler
when 'show empty' is enabled as the list will typically flash twice during
the subsequent removal and re-addition of 1k items.
The list comprehension + the explicitly parameterized memory constructor
simply aid in the optimization but do require profiling to measure the
impact. I will post my breakdown of perf. timings with the follow up
commit as I do not have them in front of me at the moment.
> - rf.memory_bounds = (0, len(self.memories))
> > + rf.memory_bounds = (0, len(self.memories)-1)
>
> This is unrelated to performance I'm sure. Is this really a bug, or does
> it matter?
>
> This is a bug and shows up when you have empty memories enabled, in which
the memories start a 1 instead of 0.
> > diff --git a/chirp/drivers/ic2200.py b/chirp/drivers/ic2200.py
> > --- a/chirp/drivers/ic2200.py
> > +++ b/chirp/drivers/ic2200.py
> > @@ -217,7 +217,6 @@
> > return mem
> >
> > def get_memories(self, lo=0, hi=199):
> > -
>
> Big performance gain on this one? :)
>
> Please remove this random whitespace damage.
>
Ack.
> > def set_memory(self, mem):
> > diff --git a/chirp/ui/bandplans.py b/chirp/ui/bandplans.py
> > --- a/chirp/ui/bandplans.py
> > +++ b/chirp/ui/bandplans.py
> > @@ -47,7 +47,7 @@
> > # Check for duplicates.
> > duplicates = [x for x in plan.BANDS if x == band]
> > if len(duplicates) > 1:
> > - LOG.warn("Bandplan %s has duplicates %s" %
> > + LOG.info("Bandplan %s has duplicates %s" %
> > (name, duplicates))
>
> Also unrelated to performance, and not a good improvement, IMHO, as it
> just makes this more chatty. Was this intentional?
>
> Technically this was not intentional, however I always make this change
locally because it reduces console spew, in that warnings are echoed to
stdout by default whereas infos are not. Probably not the best way to get
rid of this spam, but it works for me; nonetheless I can revert.
> > --- a/chirp/ui/memedit.py
> > +++ b/chirp/ui/memedit.py
> > @@ -1062,10 +1062,7 @@
> > if not mem.empty or self.show_empty:
> > gobject.idle_add(self.set_memory, mem)
> > else:
> > - mem = chirp_common.Memory()
> > - mem.number = number
> > - mem.name = "ERROR"
> > - mem.empty = True
> > + mem = chirp_common.Memory(number, True, "Error")
>
> This is okay, but presumably just a cleanup, right?
>
> This was a combination of cleanup and subsequent taking advantage of the
optimization pattern mentioned previosly.
> Thanks!
>
> --Dan
>
> _______________________________________________
> chirp_devel mailing list
> chirp_devel at intrepid.danplanet.com
> http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
> Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://intrepid.danplanet.com/pipermail/chirp_devel/attachments/20210613/2a528d99/attachment-0001.html
More information about the chirp_devel
mailing list