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

Enable interprocedural optimization #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Komzpa
Copy link

@Komzpa Komzpa commented Mar 26, 2020

without inlining:

kom@nucat:~/proj/h3/build$ bin/benchmarkH3Api 
        -- geoToH3: 1.187343 microseconds per iteration (10000 iterations)
        -- h3ToGeo: 0.796665 microseconds per iteration (10000 iterations)
        -- h3ToGeoBoundary: 2.219408 microseconds per iteration (10000 iterations)

with inlining:

kom@nucat:~/proj/h3/build$ bin/benchmarkH3Api
        -- geoToH3: 0.447104 microseconds per iteration (10000 iterations)
        -- h3ToGeo: 0.265080 microseconds per iteration (10000 iterations)
        -- h3ToGeoBoundary: 1.341663 microseconds per iteration (10000 iterations)

Addresses #325.

@isaacbrodsky
Copy link
Collaborator

Thanks for the PR! Could you check how these flags compare to configuring CMake with -DCMAKE_BUILD_TYPE=Release?

It might be necessary to place these compile flags in the if(NOT WIN32) block starting on the next line if there are incompatabilities with MSVC. Would it make sense to enable these flags only for Release builds?

@Komzpa
Copy link
Author

Komzpa commented Mar 26, 2020

master on -DCMAKE_BUILD_TYPE=Release:

kom@nucat:~/proj/h3/build$ bin/benchmarkH3Api
        -- geoToH3: 0.517386 microseconds per iteration (10000 iterations)
        -- h3ToGeo: 0.295042 microseconds per iteration (10000 iterations)
        -- h3ToGeoBoundary: 1.496694 microseconds per iteration (10000 iterations)

with this PR:

kom@nucat:~/proj/h3/build$ bin/benchmarkH3Api
        -- geoToH3: 0.432304 microseconds per iteration (10000 iterations)
        -- h3ToGeo: 0.257581 microseconds per iteration (10000 iterations)
        -- h3ToGeoBoundary: 1.376957 microseconds per iteration (10000 iterations)

Things get inlined this way, but with this PR it's somehow even faster. Seems -DCMAKE_BUILD_TYPE=Release is a secret black magic that needs to be at least documented, and defaulted if possible. At least zachasme/h3-pg#23 got hit by it, and likely many others.

@nrabinowitz
Copy link
Collaborator

Hi @Komzpa - do you plan to debug the test failures in this diff?

@Komzpa
Copy link
Author

Komzpa commented Apr 10, 2020

@nrabinowitz feel free to take over it. I currently can't allocate a large chunk of attention to give an estimate when I'll look at it next.

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

Successfully merging this pull request may close these issues.

None yet

3 participants