Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimise usage of pow using fast equivalent and exp2 #7548

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Rossmaxx
Copy link
Contributor

No description provided.

@LostRobotMusic
Copy link
Contributor

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.)

@LostRobotMusic LostRobotMusic removed their request for review October 17, 2024 15:18
@DomClark
Copy link
Member

DomClark commented Oct 17, 2024

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
Copy link
Contributor Author

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 Rossmaxx changed the title Optimise usage of pow using fast equivalents and exp2f Optimise usage of pow using fast equivalent and exp2 Oct 24, 2024
Copy link
Member

@messmerd messmerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't tested it, but it looks good

@Rossmaxx
Copy link
Contributor Author

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

@sakertooth
Copy link
Contributor

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants