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

Optimization sine/cosine functions. #304

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@gamerforEA
Contributor

gamerforEA commented Feb 12, 2015

Using sin/cos lookup table.

Benchmark result:
FastMath: 832249-843543 scores
QuickMath: 882158-885743 scores

Middle SPS on rendering:
FastMath: ~807000 SPS
QuickMath: ~850000 SPS

@gamerforEA

This comment has been minimized.

Show comment
Hide comment
@gamerforEA

gamerforEA Feb 12, 2015

Contributor

Hmmm, old comments saved... Strangely.

Contributor

gamerforEA commented Feb 12, 2015

Hmmm, old comments saved... Strangely.

@llbit

This comment has been minimized.

Show comment
Hide comment
@llbit

llbit Mar 2, 2017

Owner

I have still not benchmarked this to see if it works well in practice. The large lookup tables could blow up the CPU cache. Sometimes it's better to recompute instead of waiting for memory loads.

The precision of the sine/cosine functions is not super important, so it does not matter if they compute the most precise value, and it would be nice to replace the Apache math library. On the other hand it is generally not a good idea to maintain your own math functions - better to leave that to a well tested library.

Owner

llbit commented Mar 2, 2017

I have still not benchmarked this to see if it works well in practice. The large lookup tables could blow up the CPU cache. Sometimes it's better to recompute instead of waiting for memory loads.

The precision of the sine/cosine functions is not super important, so it does not matter if they compute the most precise value, and it would be nice to replace the Apache math library. On the other hand it is generally not a good idea to maintain your own math functions - better to leave that to a well tested library.

@llbit

This comment has been minimized.

Show comment
Hide comment
@llbit

llbit Mar 4, 2017

Owner

I tried to manually merge this pull request, but can't download the patch from GitHub:

$ curl -L https://github.com/llbit/chunky/pull/304.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   138    0   138    0     0    215      0 --:--:-- --:--:-- --:--:--   215
100    32    0    32    0     0     24      0 --:--:--  0:00:01 --:--:--   205
Sorry, this diff is unavailable.

The FastMath class is used in many more places in Chunky now than when this pull request was created, and not only the sine and cosine functions are used - FastMath.pow() is also used for example. I don't really want to go through and edit all the sine/cosine uses to benchmark this pull request now. As I said in my previous comment I have some doubts that this would actually improve performance. However, that is moot since I don't seem to be able to merge this pull request at all, and I would have to rewrite the whole thing manually.

Please feel free to make a new pull request with the changes based on the current version of Chunky. I will make sure to benchmark it quickly if you create a new pull request, so that it doesn't take years to resolve the pull request as in this case.

Owner

llbit commented Mar 4, 2017

I tried to manually merge this pull request, but can't download the patch from GitHub:

$ curl -L https://github.com/llbit/chunky/pull/304.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   138    0   138    0     0    215      0 --:--:-- --:--:-- --:--:--   215
100    32    0    32    0     0     24      0 --:--:--  0:00:01 --:--:--   205
Sorry, this diff is unavailable.

The FastMath class is used in many more places in Chunky now than when this pull request was created, and not only the sine and cosine functions are used - FastMath.pow() is also used for example. I don't really want to go through and edit all the sine/cosine uses to benchmark this pull request now. As I said in my previous comment I have some doubts that this would actually improve performance. However, that is moot since I don't seem to be able to merge this pull request at all, and I would have to rewrite the whole thing manually.

Please feel free to make a new pull request with the changes based on the current version of Chunky. I will make sure to benchmark it quickly if you create a new pull request, so that it doesn't take years to resolve the pull request as in this case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment