BundleToResources adjustment and optimization#543
BundleToResources adjustment and optimization#543brynrhodes merged 15 commits intocqframework:masterfrom
Conversation
… to do, checking in progress.
…p processing of files (not json or xml, or matches output path)
…ils to avoid memory issues on macos
…after the bundle, but folder structure of where files originate from is retained.
…o run use -db=true, updated test with new delete feature.
…tem. Also correcting an issue where a reused instance of BundleToResources() was erroneously passing in -db=true to a test that shouldn't have it.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #543 +/- ##
============================================
+ Coverage 21.00% 21.17% +0.16%
- Complexity 1656 1666 +10
============================================
Files 298 298
Lines 26121 26218 +97
Branches 4149 4168 +19
============================================
+ Hits 5488 5552 +64
- Misses 19737 19756 +19
- Partials 896 910 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@echicoine-icf What is the issue you found with the ThreadUtils usage? |
I found with larger operations, like refreshing the ecqm-content-qicore-2024 repo (located here https://github.com/cqframework/ecqm-content-qicore-2024) my mac seemed to be overloaded by the system-managed cached thread pool. In Windows, it works great (and when I wrote the class, I did so on my PC.) I read up on it, turns out this is a common observation and it boils down to unix based operating systems trusting developers more in managing threads etc. Limiting the thread pool count prevents a unix system from being overburdened to the point of locking up and becoming unusable. |
|
Yeah that makes sense. Your work on the bundles looks good but we need to do something a bit different with the threading changes. There is a concern that the ThreadUtils class is used across other features and we haven't seen other users report issues. Nor has our team experienced these issues. That being said, to move the PR forward here are 2 options
Using a flag will allow users that haven't experienced the same issues as you have won't experience and changes. Even if option 1 is used we'll still want to see a flag to allow users to choice to override the cached model. |
|
When I wrote the class it was to help with performance during the RefreshIG operation, but I also implemented it into ExtractMatBundle. So far these are the only two operations that utilize it so I won't be stepping on anyone's toes so to speak. As far as users reporting the issue, I think it's likely due to these operations not getting used much or getting used by Windows users. This work is to try to get out ahead of the user complaints. I do think it's a good idea thought to back out the change to this file and create a standalone PR for better tracking. I don't want to introduce any new flags because I feel like this should just work seamlessly for all users without them having to tweak anything. |
|
new PR for ThreadUtils here: #555 |
|
Thanks for pulling out the ThreadUtils change. |
Changes:
https://icf.atlassian.net/browse/CQF-2407