Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#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 Lennart Ochel, 10 years ago

Cc: Lennart Ochel added

comment:2 by Willi Braun, 10 years ago

Cc: Willi Braun added

Okay, good to know. Thanks for the hint!

comment:3 by Per Östlund, 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 Per Östlund, 10 years ago

Resolution: fixed
Status: newclosed

in reply to:  3 ; comment:5 by Martin Sjölund, 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.

in reply to:  5 comment:6 by Per Östlund, 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 Martin Sjölund, 10 years ago

Resolution: fixed
Status: closedreopened

comment:8 by Martin Sjölund, 10 years ago

Owner: changed from someonewithenoughtime to Per Östlund
Status: reopenedassigned

comment:9 by Martin Sjölund, 10 years ago

Resolution: fixed
Status: assignedclosed

comment:10 by Dietmar Winkler, 9 years ago

Milestone: Futurepre1.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 Martin Sjölund, 7 years ago

Milestone: pre1.9.41.9.4

Removing the pre1.9.4 milestone in favor of 1.9.4.

Note: See TracTickets for help on using tickets.