LMMS / lmms

Cross-platform music production software
https://lmms.io
GNU General Public License v2.0
8.18k stars 1.01k forks source link

Optimise usage of pow using fast equivalent and exp2 #7548

Closed Rossmaxx closed 1 day ago

LostRobotMusic commented 1 month ago

Don't use fastPow in these cases, that approximation is extremely inaccurate and should only be used when precision isn't required. This is likely a major backwards compatibility break, always perform null tests with these things.

(Note that fastPow10 in contrast doesn't use an approximation and is therefore perfectly safe, despite the similar name.)

DomClark commented 1 month ago

I'd rather we used std::exp2, which will operate at the appropriate precision for its argument, than std::exp2f, which causes its argument to be implicitly converted to a float.

Also, in addition to Lost Robot's comment on the inaccuracy of the fastPow approximation, I would add that a lot of these cases are actually in UI code or file IO code, where performance doesn't matter nearly as much, and the cost of the computation is most likely dwarfed by other operations. As well as testing for regressions in behaviour, you should also profile the application before and after the changes to ensure we actually see a gain in performance in compensation for the reduction in accuracy.

Rossmaxx commented 1 month ago

I'd rather we used std::exp2,

Noted

, in addition to Lost Robot's comment on the inaccuracy of the fastPow approximation, I would add that a lot of these cases are actually in UI code or file IO code, where performance doesn't matter nearly as much, and the cost of the computation is most likely dwarfed by other operations.

I have reverted the of calls to fastPow. What exists now is calls to fastPow10f (mind the 10f). That function is accurate AND fast.

As well as testing for regressions in behaviour, you should also profile the application before and after the changes to ensure we actually see a gain in performance in compensation for the reduction in accuracy.

I don't have profiling set up in this branch. I'll try something tho.

Rossmaxx commented 3 weeks ago

I couldn't see much performance improvement from profiling. @sakertooth do we still want this. Atleast there's no change in behaviour.

sakertooth commented 3 weeks ago

I couldn't see much performance improvement from profiling. @sakertooth do we still want this. Atleast there's no change in behaviour.

I generally always prefer using what is provided in the standard library by default. So, if the optimized pow function is not needed here, I personally would not want us to make the switch.

Rossmaxx commented 3 weeks ago

In some places, the updated pow function is needed but in other places, not really, but I would say, change everything as fastPow10f is accurate enough for the purpose unlike the other fastPow.

Rossmaxx commented 1 week ago

@sakertooth we fine with pushing this as is, or should I enforce the std::pow ?

sakertooth commented 1 week ago

My stance is the same still: I would prefer std::pow if something else isn't strictly necessary, but I am not entirely sure how others feel about this. @DomClark mentioned that most of these changes are within the UI code, where the performance gains from using something like a fast pow approximation is dwarfed by other operations that happen there, so I think using std::pow is the way to go here.

Rossmaxx commented 1 week ago

Should I revert the ones in monstro and Drumsynth too?

Rossmaxx commented 5 days ago

I didn't get an answer here. Even if i should make the change, I can't for now because my laptop display is gone (posted that story on discord).

I'm just a bit too enthusiastic to hit that button.

sakertooth commented 5 days ago

Apologies for the delay, I'm reviewing this right now @Rossmaxx.