[chirp_devel] [PATCH] [csv] Optimize generic csv files load times. Fixes #8991

[Dan Smith]

> # 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.

>  @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.

> -    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)?

> -        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?

> 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.

>      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?

> --- 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?

Thanks!

--Dan