#2947 closed defect (fixed)
Performance issues with BackendVariable.mergeVariables
Reported by: | Martin Sjölund | Owned by: | Per Östlund |
---|---|---|---|
Priority: | high | Milestone: | 1.9.4 |
Component: | Backend | Version: | trunk |
Keywords: | Cc: | Per Östlund, Martin Sjölund, Lennart Ochel, Willi Braun |
Description
BackendVariable.mergeVariables has huge performance issues. One way to fix them would be to make the VARIABLES() structure actually a list of VARIABLES() to search. Or to optimize the merging.
There are many "odd" calls to convert list<Var> to Variables that are then not used (only merged).
SymbolicJacobian.getSymbolicJacobian costs ~22% of execution time in the 4-bit adder. Most of that is mergeVariables.
Change History (11)
comment:1 by , 10 years ago
Cc: | added |
---|
comment:2 by , 10 years ago
Cc: | added |
---|
follow-up: 5 comment:3 by , 10 years ago
Fixed in r23275. Previous performance for the 4-bit adder as reported by simulate():
timeBackend = 27.938798641, timeSimCode = 18.322192011,
After my changes:
timeBackend = 13.961148169, timeSimCode = 9.671017581999999,
Unfortunately it doesn't seem to have much of an impact on the testsuite as a whole, although it's a bit faster. I will see what Hudson indicates, maybe I've made something else slower.
I started out by changing VARIABLES into a list of variable sets, but in the end I reverted back to a single set since having a list of sets wasn't as useful as I thought it would be. The performance improvements are due to optimizing the functions that handle VARIABLES, as well as using those functions smarter.
comment:4 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
follow-up: 6 comment:5 by , 10 years ago
Replying to perost:
Unfortunately it doesn't seem to have much of an impact on the testsuite as a whole
Before:
Slowest test 428.472s Spice3BenchmarkFourBitBinaryAdder
15644.80user 1096.62system 7:23.01elapsed 3778%CPU (0avgtext+0avgdata 3693736maxresident)k
608inputs+535840outputs (0major+330834515minor)pagefaults 0swaps
Now:
Slowest test 406.324s HeatingSystem
15469.00user 1107.14system 7:15.80elapsed 3803%CPU (0avgtext+0avgdata 3622772maxresident)k
Seems good. Got a bit more CPU-time this run. But it seems to be a "big" boost.
Before switching to bootstrapped:
https://test.openmodelica.org/hudson/view/Linux/job/OpenModelica_TEST_CLANG/9062/console
Slowest test 407.936s HeatingSystem
13543.69user 1552.20system 6:53.53elapsed 3650%CPU (0avgtext+0avgdata 2352240maxresident)k
25576inputs+532200outputs (0major+350273543minor)pagefaults 0swaps
We are already faster when it comes to multi-threaded simulation. Soon we will be faster than RML when running using a single GC marker.
comment:6 by , 10 years ago
Replying to sjoelund.se:
Slowest test 406.324s HeatingSystem
buildModel of HeatingSystem takes ~13 sec for me, the rest is simulation. So I guess it's only very large models that benefit from my changes, and we don't have a lot of those.
Anyway, some things I forgot to mention:
- BackendVariable.listVar adds the variables to the hashtable in reverse order, i.e. it does listReverse on the list first. This is weird and wasteful, but removing it breaks a lot of tests.
- BackendDAEUtil.calculateValues does knvars := listVar(varList(knvars)) to reverse the list of known variables. Again, seems stupid, but not doing it breaks stuff.
It would be nice if anyone who knows more about the backend could figure out why this is needed, and maybe fix it. But it doesn't seem to impact the performance much.
comment:7 by , 10 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:8 by , 10 years ago
Owner: | changed from | to
---|---|
Status: | reopened → assigned |
comment:9 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:10 by , 9 years ago
Milestone: | Future → pre1.9.4 |
---|
It doesn't make sense to keep closed ticket in the "Future" milestone that were simply forgotten to assign to the correct milestone in the past.
comment:11 by , 7 years ago
Milestone: | pre1.9.4 → 1.9.4 |
---|
Removing the pre1.9.4 milestone in favor of 1.9.4.
Okay, good to know. Thanks for the hint!