sys/frac: Integer fractions #9283
Merged
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
jnohlgard
added
State: WIP
|
I feel that this is a superior approach to what I was working on in #9280. I was considering fractions rather than simple reciprocals as one method to fix the precision problem with 512/15625 (which results in a major loss in precision in #9280 currently). Timer tick conversion was also the main reason I rewrote sys/div.h. In my opinion, if this PR works well for the task then it should supersede #9280, and sys/div.h in general. |
|
Div is still faster during execution, but with the drawback that the fraction must be a compile time constant. |
|
Here is some output from the benchmark test. I had to change TEST_NUMOF to 512 to get it to fit in ram. It would be interesting to see the difference between the old div behavior and #9280. |
This was referenced Jun 6, 2018
|
It is surprising to see that the division operator is so much faster than the other methods on atmega. Do they have hardware division? Here are some numbers for mulle (Cortex-M4), note how the relative performance between the different methods is reversed compared to atmega: Edit: Updated with float test |
|
and some numbers for frdm-kw41z (Cortex-M0+), note that the compiler bundled division function is significantly slower here than on CM4: Edit: Updated output to include float test |
|
ATmega does have some support for division: FMUL - Fractional Multiply Unsigned, FMULS - Fractional Multiply Signed, FMULSU - Fractional Multiply Signed with Unsigned. I had not noticed this before. Neat! |
|
Added one more benchmark for a scaling via 64 bit float. It is a lot slower on CM0+, but fast on big CPUs. |
|
@ZetaR60 btw, do the unittests for tests-frac and tests-div pass on atmega? |
|
@gebart The partial output of tests-frac is below. tests-div passes. There seems to be a bug in printf when trying to print 64-bit numbers (I had a similar issue when working on div) |
|
Might be an instance of #1891 |
|
This implementation needs some work. To do items:
|
|
The planned changes will make this implementation similar to what an optimized |
|
it might be easier to merge this if changes to xtimer are introduces in a separate PR. First having the frac module in without touching existing code will be safer - though I see the point in putting this to (good) use right away. |
|
@smlng you are right. The unit tests should be enough in this PR |
jnohlgard
force-pushed
the
pr/frac
branch
2 times, most recently
from
443090f
to
24315b2
Compare
The rounding errors occur when the implicit product (numerator * unscaled) is large (> 2**31). In comparison, libdivide performs some corrections for certain inputs to adjust the result, and I believe this observed rounding error is related to those adjustments. These adjustments have not been implemented here (both because of lacking time, and not fully understanding the mathematical proof). Some useful papers for this topic, by one of the principal authors of GMP: |
jnohlgard
force-pushed
the
pr/frac
branch
2 times, most recently
from
246563b
to
2ea5f4f
Compare
|
Hi @gebart, here some test output run on STM32 Bluepill, it looks a bit weird but also regular with those periodic bursts of UINT32_MAX (4 trillion something) which I don't see in your sample output. |
|
@smlng I noticed the same thing on the samr21-xpro. The big numbers happen if xtimer_now has moved backwards during the test execution. I think there is a problem with the timer driver on the Samr21, this shows in bench_timers too, I didn't look into the stm32 implementation |
kaspar030
reviewed
Sep 5, 2018
|
@smlng are you still interested by this PR (since you started to review it) ? |
|
FIY, this module is used in a WIP ztimer module that implements dynamic frequency scaling (using frac for arbitrary fractions). I think it is really useful. Not sure @gebart will work on this, so we'll probably have to adopt this. |
|
Until then, let's reopen to keep us reminded of that fact ;-) |
stale
bot
removed
the
State: stale
kaspar030
added
CI: ready for build
I just did. I've saved the benchmark commits locally and pushed a rebased version of this branch without the benchmark. |
|
As a summary to newcomers: This PR is basically an enhanced version of our div module. Whereas the latter allows division by multiplication with magic numbers, this PR allows the same but with saving the necessary reciprocal/shift in a struct (instead of requiring static defines like div does), which can optionally be calculated at runtime. The use case is calculating semi-constant fractions (and avoiding using float division). That can be used in, e.g., timers that need to change their conversion factor at run-time in order to synchronize over network. |
kaspar030
reviewed
Dec 3, 2019
kaspar030
reviewed
Dec 3, 2019
maribu
reviewed
Dec 3, 2019
sys/frac/frac.c
Outdated
| * then set v = v - u (which is even). */ | ||
| if (u > v) { | ||
| /* Swap u and v */ | ||
| unsigned int t = v; |
There was a problem hiding this comment.
Possible loss of precision, as v is uint32_t and unsigned int can be 16 bits wide
kaspar030
added
the
CI: run tests
kaspar030
approved these changes
Dec 5, 2019
|
Ups, I had pushed the latest doc & test fixes to the wrong upstream. Fixed. |
|
@smlng Could you take a look? Your issues where with the banchmark application which is now gone. |
aabadie
reviewed
Dec 5, 2019
|
Let's go. Thanks everyone! |
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.
Contribution description
Introduces the frac library for integer scaling by semi-constant fractions. The implementation is based around the libdivide integer division library (https://libdivide.com) which uses the multiplicative inverse to compute integer division. The parameters required can be computed at runtime, or optionally pre-configured at compile time. A helper application (tests/frac-config) is provided to generate static configurations.
A benchmark application (tests/bench_frac_div) is provided to compare the different division methods (div module, frac module, division operator), both using constant fractions and runtime variable fractions.
The intended use case is a better timer algorithm with runtime adjustable tick conversion frequency for improved clock synchronization between nodes.
Libdivide is imported as a copy from the official repo, instead of as a git pkg, since the library is a single header file and I believe this functionality will become quite central to many applications and we don't want to require a network connection to build this. This will be refactored into a pkg with a locally cached copy of the header.
Issues/PRs references
Similar but different use case: #9280