Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#2947 closed defect (fixed)

Performance issues with BackendVariable.mergeVariables

Reported by: sjoelund.se Owned by: perost
Priority: high Milestone: 1.9.4
Component: Backend Version: trunk
Keywords: Cc: perost, sjoelund.se, lochel, wbraun

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 Changed 10 years ago by lochel

  • Cc lochel added

comment:2 Changed 10 years ago by wbraun

  • Cc wbraun added

Okay, good to know. Thanks for the hint!

comment:3 follow-up: Changed 10 years ago by perost

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 Changed 10 years ago by perost

  • Resolution set to fixed
  • Status changed from new to closed

comment:5 in reply to: ↑ 3 ; follow-up: Changed 10 years ago by sjoelund.se

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 in reply to: ↑ 5 Changed 10 years ago by perost

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 Changed 10 years ago by sjoelund.se

  • Resolution fixed deleted
  • Status changed from closed to reopened

comment:8 Changed 10 years ago by sjoelund.se

  • Owner changed from someonewithenoughtime to perost
  • Status changed from reopened to assigned

comment:9 Changed 10 years ago by sjoelund.se

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:10 Changed 9 years ago by dietmarw

  • Milestone changed from Future to 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 Changed 7 years ago by sjoelund.se

  • Milestone changed from pre1.9.4 to 1.9.4

Removing the pre1.9.4 milestone in favor of 1.9.4.

Note: See TracTickets for help on using tickets.