search

mezuro / kalibro_processor

Reimplementation of Kalibro processing
GNU Affero General Public License v3.0
2 stars 10 forks source link

Optimize TreeMetricResult#descendant_values #224

Closed rafamanzo closed 8 years ago

rafamanzo commented 8 years ago

The previous implementation was incredibly sub-optimal. It can be easily replaced with a join on the parent_id.

Aggregation performance test results below:

Metric Count Branch Wall Time (s) Process Time (s)
1 v1.3.2 34.16732797622681 29.564079139199997
1 improve_descendant_values 23.588372707366943 21.618301714000005
2 v1.3.2 71.57923197746277 63.06931912939999
2 improve_descendant_values 47.487600564956665 46.2433640588
4 v1.3.2 154.1164906024933 147.8050995992
4 improve_descendant_values 97.7507544517517 70.0752864698
8 v1.3.2 333.3348692417145 311.43665788299995
8 improve_descendant_values 190.28073539733887 226.78861793199994

@mezuro/core we need a third pair of eyes here as well.

This is part of https://github.com/mezuro/kalibro_processor/issues/207.

danielkza commented 8 years ago

Here are some tests on my machine, including max. resident RAM (all using 8 metrics). Files are named for the respective branches. Gist.

In short, this is a really (almost 100%) performance good improvement by itself, but still quite a bit worse than the full aggregation rewrite (which actually doesn't even use descendant_values anymore!).

:+1: for me

rafamanzo commented 8 years ago

Even tough this will have no impact on aggregation after https://github.com/mezuro/kalibro_processor/pull/225, it will still be used for MetricResultsController and the code legibility improvement is nice as well.

Nice job!

(I've removed some code made useless by this)

danielkza commented 8 years ago

Right, I missed that. Nice catch on the useless code!

diegoamc commented 8 years ago

Great job, guys!