speed up util::normalize_angle for stupidly-large values #3337
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
include/mapnik/util/math.hpp
Outdated
| namespace mapnik { namespace util { | ||
|
|
||
| constexpr double pi = 3.14159265358979323846; | ||
|
|
There was a problem hiding this comment.
M_PI can be used from mapnik/global.hpp
There was a problem hiding this comment.
M_PI is not part of c++ so yes, defining it in include/mapnik/util/math.hpp and removing MSVC related #if/def in global.hpp makes sense to me. One thing to be aware third party libs are using M_PI from math.h, agg has its own #define ... We just have to be consistent
This partially reverts commit 5ab6db2. The change of argument --threads N doesn't just change output, it changes the mode of operation. With --threads 0, the benchmark runs wholly in the main thread. With --threads 1, it starts 1 worker thread, which does the work, and the main thread measures how long it takes to complete.
Make it clear in benchmark output which mode it ran in: - "main thread" only - "1 worker" thread - "N workers"
Because it simply calculates the remainder after division by full turn, it shouldn't take time proportional to the magnitude of its operand.
Codecov Report
@@ Coverage Diff @@
## master #3337 +/- ##
=========================================
- Coverage 68.9% 68.9% -0.01%
=========================================
Files 442 442
Lines 23259 23263 +4
=========================================
+ Hits 16027 16029 +2
- Misses 7232 7234 +2
Continue to review full report at Codecov.
|
leedejun
pushed a commit
to leedejun/mapnik
that referenced
this pull request
Dec 26, 2023
speed up util::normalize_angle for large values
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
It's pretty unlikely that real code calls this with stupidly large angles (like 100 radians), but I think these should be handled gracefully, too.
In my benchmarks the new function is
The point at which
std::remainderis faster than subtracting2 * piin a loop is of course highly architecture dependent. I haven't done any real investigation -- just ran a simple benchmark,std::remainderstarted winning around 110 radians on my laptop. So I picked 100 as the point where the extra precision might be worth it -- despite the fact that my function is still slower below ~130, because it does a few subtraction loops that are thrown away if the angle is too large.