ENH: Refactor solver and improve solver performance

[bocklund]

I have noticed in benchmarking that roughly 1/3 of the time in the solver is spent performing this copy operation on the state. We don't do anything meaningful with the old state except for using its data, so I propose to remove it and replace the couple instances with explicitly storing the relevant variables before taking a step.

If we want to do more advanced things with the previous state(s) later, that's definitely something we could do, but I think all the memory allocations in one of the innermost loops is probably not worth it compared to copying the values we need out.

This change could probably use a more real benchmark to justify it, but I wanted to get it posted for feedback before I invest much more time into it.

[bocklund]

An alternative to this could be to just have one old_state where these quantities are copied in using a similar pattern instead of copying every iteration.

[codecov]

Codecov Report

Merging #360 (b422fef) into develop (f5bd032) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop     #360   +/-   ##
========================================
  Coverage    89.75%   89.75%           
========================================
  Files           44       44           
  Lines         4275     4275           
========================================
  Hits          3837     3837           
  Misses         438      438           

Impacted Files	Coverage Δ
pycalphad/plot/binary/map.py	86.66% <100.00%> (ø)
pycalphad/io/tdb_keywords.py	100.00% <0.00%> (ø)

---

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 f5bd032...b422fef. Read the comment docs.

[richardotis]

This approach is fine.

[bocklund]

Below are some profiling runs from a 10 iteration ESPEI run of a binary system (with Cython profile directive enabled).

These changes give ~30% performance improvement. I did a couple of runs with Cython profiling on and off and the improvement in the find_solution timings is repeatable.

I also tried annotating the Cythonized output and playing with some Cython optimizing directives, which didn't seem to make much measurable difference.

Develop

This branch

In the fourth row, the fourth box to the right are calls to np.all (14 seconds in total), so there's a little to squeeze out, but there are probably hotter spots to optimize still.

[richardotis]

Wow, copying SystemState objects is really expensive. 30% performance improvement for such a small change definitely sounds good to me.

[bocklund]

Copying the state is about half the overall speedup (it's the labeled box in the fourth row of the develop branch benchmarks, accounting for 82 seconds). The other half comes from refactoring the allocations and calls to np.max where the lazier np.all or np.any calls give the same result.

But yeah, almost 20% of the current solver time is just copying the State object

[bocklund]

Fresh benchmarks on e20464f

[bocklund]

The bigger step size and new Gibbs phase rule code calls take_step about half as many times as the previous benchmark run, so that's why the total find_solution time is about 70% faster in the most recent commit compared to my previous benchmark.

[bocklund]

The benchmark above was for a binary case. I also ran a benchmark equilibrium calculation in a 5 component system and can verify the performance holds up compared to develop where we still see a 2.6x overall improvement.

In the 5 component benchmark, each call to take_step is about 25% faster than develop, but there are almost 2x as many steps on average to find the solution, leading to a 50% slowdown in the time per find_solution. However, find_solution is called only 400 times compared to 1800 (a 4x decrease), so we are taking half as many steps in total here compared to develop. I attribute this behavior largely to having more metastable candidates and phase cycling in the 5 component case, but fewer overall calls to find_solution because the true stable composition sets likely make it into the candidates earlier.

[bocklund]

I noticed that map_binary still has pdens set to 2000. The _solve_eq_at_conditions has a performance regression here due to the new code added, which goes away almost completely by lowering the pdens down to 50. Since users can pass higher point densities, this might be something to look into before we merge.

Lowering to 50 gives no noticeable regression in the plotting except for some narrow regions are missed (proper mapping will solve this), so it should be safe to do.

[richardotis]

Great work!