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.
> # HG changeset patch
> # User Kosta A. <ve7kcy@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@intrepid.danplanet.com
http://intrepid.danplanet.com/mailman/listinfo/chirp_devel
Developer docs: http://chirp.danplanet.com/projects/chirp/wiki/Developers