pcdshub / pytmc

Generate EPICS IOCs and records from TwinCAT projects - along with many TwinCAT project tools
https://pcdshub.github.io/pytmc/
Other
10 stars 11 forks source link

PERF: reduce amount of deepcopy calls in build_singular_chains #83

Closed klauer closed 5 years ago

klauer commented 5 years ago

This appears to be at the core of https://github.com/slaclab/pytmc/issues/82.

It was not an infinite loop as I had guessed, but it was taking on the order of minutes and gigabytes of memory due to unnecessary deepcopy calls.

With the current master, I gave up after 2 minutes of waiting:

image

With this PR, it goes at a not-so-unreasonable speed:

$ time pytmc --allow-errors VonHamos01.tmc test.db

real    0m9.965s
user    0m9.776s
sys     0m0.157s
codecov-io commented 5 years ago

Codecov Report

Merging #83 into master will decrease coverage by 0.8%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
- Coverage   92.05%   91.25%   -0.81%     
==========================================
  Files           6        6              
  Lines         881      880       -1     
==========================================
- Hits          811      803       -8     
- Misses         70       77       +7
Impacted Files Coverage Δ
pytmc/xml_obj.py 92.74% <100%> (-0.29%) :arrow_down:
pytmc/xml_collector.py 96.28% <100%> (-1.5%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7eb1727...a68b549. Read the comment docs.

klauer commented 5 years ago

Surprisingly, caching the name with what I believe the intended mechanism is (in fda21c0) shaved a few seconds off:

real    0m6.622s
user    0m6.401s
sys     0m0.173s

And another second off in not unnecessarily calling str(entry) in a68b549:

real    0m5.345s
user    0m5.133s
sys     0m0.171s
klauer commented 5 years ago

Tests are apparently not checking what I mentioned above; do not merge yet.