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

Compiler optimizations #494

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

Compiler optimizations #494

wants to merge 5 commits into from

Conversation

bkj
Copy link

@bkj bkj commented Sep 29, 2021

This pull request includes two simple compiler optimizations:

  • a) Use -O3 -ffast-math in CMakeLists.txt
  • b) Inline the sqr(x) {return x * x} function from lib/utils.cpp

Testing

I've also added a build and benchmark script, so we can run w/:

./build_and_run.sh

Results

version burnin posterior sampling
original code 9.5s 1.8s
a only 3.7s 0.7s
a and b 1.2s 0.2s

So using both optimizations reduces runtime of this example by ~87%.

Notes

This is a PR to master, but the numbers and experiments above are based on comparison to commit c07eeae9394ab30ca8d984b2ec2e40ab4c2d2e08, per @manujinda 's recommendation. I have not tested on master, but it should be easy for you to copy/paste these changes to whichever branch is currently under development.

Possible Future Work

After these optimizations, profiling via callgrind reveals that the majority of runtime is spent computing calls to exp in KDE::pdf. To get additional speedups, we'd need to make exp run faster. This may be possible, depending on how much precision is required, by eg. using approximate exp functions like the ones described here.

@bkj
Copy link
Author

bkj commented Oct 1, 2021

Testing this some more -- it's possible that -ffast-math actually breaks something. If/before this gets merged -- the impact of -ffast-math should be tested. Speedup w/o -ffast-math is still substantial (2.9s).

@adarshp
Copy link
Collaborator

adarshp commented Oct 7, 2021

I'd like to request a couple of changes, just to keep things organized - can the 'test.py' and 'build_and_run.sh' scripts be moved to the 'scripts' directory?

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.

2 participants