Use multiprecision rational arithmetic to propagate tags #1543

awalls-cx18 · 2018-01-05T22:23:52Z

To address the problem, pointed out by Eugene Grayver, in these threads:

https://lists.gnu.org/archive/html/discuss-gnuradio/2017-11/msg00079.html
https://lists.gnu.org/archive/html/discuss-gnuradio/2017-12/msg00131.html

Avoid floating point precision problems in propagation of tags for the following cases:

Common relative rates that are non-terminating in binary (1/3, 1/5, 1/7, ...)
Large tag offsets
Inexact floating point reciprocals when specifying the relative rate

This change to the runtime only fixes the tag propagation problems for
blocks with a fixed/unchanging relative rate.

This change adds a dependency on the MIPR library (a fork of the GMP
library) to perform multiple precision interger rational arithmetic.
MIPR was chosen as it purports to maintain a properly working Windows
build for cross-platform compatability. Optimizations in MIPR are
also targeted to ARM in additional to x86-64.

Optimize some reference counted buffer[_reader]_sptr grabs and releases that show up as minor CPU wasters in profiling when there are a lot of tags or some blocks that "return 0" often. At high sample rates (e.g. 160 Msps), this can save ~2% CPU on blocks that propagate a fair number of tags.

Avoid floating point precision problems in propagation of tags for the following cases: - Common relative rates that are non-terminating in binary (1/3, 1/5, 1/7, ...) - Large tag offsets - Inexact floating point reciprocals when specifying the relative rate This change to the runtime only fixes the tag propagation problems for blocks with a fixed/unchanging relative rate. This change adds a dependency on the MIPR library (a fork of the GMP library) to perform multiple precision interger rational arithmetic. MIPR was chosen as it purports to maintain a properly working Windows build for cross-platform compatability. Optimizations in MIPR are also targeted to ARM in additional to x86-64.

The fractional resampler and fractional interpolator blocks' output samples would lose sync with tag propagation after a while due to floating point precision problems: - The blocks did their job with mixed float & double arithmetic to generate output samples, but the runtime used double arithmetic to propagate tags. - Floating point reciprocals are quite exact in the least significant bits, so specifying relative_rate as 1.0/resample_rate introduces a mismatch in rates.

This is a mass conversion of existing set_relative_rate(double) calls to either the set_relative_rate(uint64_t, uint64_t) or set_inverse_relative_rate(double) calls, where ever easily possible and appropriate, to improve tag propagation precision for fixed relative rates.

noc0lour · 2018-01-06T14:23:22Z

Thanks for fixing this!

Currently MPIR is not packaged in Debian/Ubuntu and as for as I read up it will never be packaged for either of them. Fedora/CentOs on the other hand have this package available. Problem with MPIR apparently were that it redefines symbols which are available in GMP and thus a library can't be linked directly/indirectly to both. Not sure if these problems are solvable in Debian/Ubuntu, maybe mait can comment on that before we pull this in as dependency. Is there any chance you can use GMP for this?

awalls-cx18 · 2018-01-06T15:23:33Z

On Sat, 2018-01-06 at 14:23 +0000, Andrej Rode wrote: Thanks for fixing this!

You're welcome. I must confess though, that like all things I fix, I needed this working for myself. :)

Currently MPIR is not packaged in Debian/Ubuntu and as for as I read up it will never be packaged for either of them.

And apparently GMP will never be ported and verified for Windows. That is why the MPIR fork of GMP exists.

Fedora/CentOs on the other hand have this package available.

Yes. The installed headers are /usr/include/mirpxx.h and /usr/include/mpir.h . The installed libraries are libmpirxx.so and libmpir.so .

Problem with MPIR apparently were that it redefines symbols which are available in GMP and thus a library can't be linked directly/indirectly to both.

I can believe that, skimming the header files.

Not sure if these problems are solvable in Debian/Ubuntu, maybe mait can comment on that before we pull this in as dependency. Is there any chance you can use GMP for this?

There are a few options, I guess: 1. Toss MPIR, and just use GMP. Windows compatability then falls on the maintainer of the Windows port. Probably not a nice thing to do to Geof. 2. Convince Debian/Ubuntu to actually package MPIR. Unlikely, I guess? 3. Add a pybombs recipe for MPIR that correctly builds on Debian/Ubuntu. This will probably exclude a distro packaged version of gnuradio being available. 4. Package MIPR as a sublibrary of GNURadio, sort of like how many GNU packages do this for readline, etc. FSF may have some heartburn with this, since GNURadio is a FSF project. 5. Conditional compilation or C++ abstraction to use MPIR on Windows and GMP on Linux. 6. Use the boost native multiprecision ipmlementation which is cross platform, but not optimized like GMP and MPIR. I'd prefer option 2, but option 5 is within my control to fix without external parties involved. I'm not that good with C++ abstraction, so if I implement option 5, I'll probably start with a conditional compilation approach.

awalls-cx18 · 2018-01-06T22:33:43Z

After some thought, I think Debian/Ubuntu made the right call. OOT module authors on Linux that use an MP library themselves will very likely pick GMP, and this patch would force them to change to MPIR due to linker problems.

I'll add a patch to look for both MPIR and GMP and will prefer to use GMP with fallback to MIPR, if GMP is not found.

…ilable

awalls-cx18 · 2018-01-07T02:43:39Z

OK, patched to search for both GMP and MIPR and to prefer GMP, if GMP is found.

noc0lour · 2018-01-07T14:11:45Z

OK, I'll need to update the build containers to include GMP so it will build and test.

…c build External applications including block.h should not have to figure out the correct #define for which MP library to use, so make the correct GR_MPLIB_* define in gnuradio/config.h at the time libgnuradio-runtime is configured and built. Also fix the generation of the pkgconfig file gnuradio-runtime.pc, so that external applications linking against libgnuradio-runtime can easily know which MP library to link against.

awalls-cx18 · 2018-01-07T23:00:33Z

Fixed gnuradio/config.h to include a define for the proper MP library and fixed generation of gnuradio-runtime.pc for the proper MP library, so in both cases external application writers need not guess.

noc0lour · 2018-01-07T23:56:42Z

-- Configuring gnuradio-runtime support...
--   Dependency Boost_FOUND = 1
--   Dependency ENABLE_VOLK = ON
--   Dependency PYTHONINTERP_FOUND = TRUE
--   Dependency MPLib_FOUND = 
--   Disabling gnuradio-runtime support.
--   Override with -DENABLE_GNURADIO_RUNTIME=ON/OFF

Somehow finding GMP is not enough, it still is not enabling gnuradio-runtime.

awalls-cx18 · 2018-01-08T00:37:44Z

Per conversation on IRC, this is a CMake 2.8.12 & and not reading the CMake documentation problem with finding a module with a mixed case name. CMake 3.10.0 doesn't fail. The chosen solution is to just change "MPLib" to "MPLIB" everywhere and make the problem go away. I'll push that change soon.

… versions

marcusmueller · 2018-02-03T13:28:51Z

To give you a bit of status insight on this PR:

We can't merge that into master right now, because of the new dependency; we'll have to discuss this. I'm not really opposed to pulling in new dependencies into 3.8, although the overall goal is to reduce dependency hell:

GMP/MPLIB/MPIR is a relatively benign dependency; however, I really need to track down availability on more than our favourite linuxes. I vaguely remember GMP and Windows having a special relationship. So, until I start pulling together the 3.8 release, this must stay unmerged (and hence, the PR stays open to force me to actually do that).

awalls-cx18 · 2018-02-03T13:55:47Z

The GMP folks specifically refuse to support a Windows port. That is why the MPIR fork of GMP exists, to have the guts of GMP work properly on Windows.

Because of this though, the symbols of GMP and MPIR clash when linking. Thus the preferred default of GMP, so that libgnuradio*.so will likely not have linking problems with user apps under Linux.

awalls-cx18 · 2018-02-03T14:15:35Z

FYI, I didn't "roll my own" routines for multiple precision integers and rationals because I figired I'd get it wrong somewhere, and I really try to avoid "not invented here" syndrome when coding.

Thus the addition of a dependency. Yeah I agree GNURadio already has too many dependencies, but I was in situation of having to choose between the lesser of two evils with this fix. :(

marcusmueller · 2018-02-11T15:40:51Z

cmake/Modules/FindMPLIB.cmake

+    set(MPLIBXX_LIBRARY ${GMPXX_LIBRARY})
+    set(MPLIB_PC_ADD_CFLAGS ${GMP_PC_ADD_CFLAGS})
+    set(MPLIB_PC_ADD_LIBS ${GMP_PC_ADD_LIBS})
+else(GMP_FOUND)


Shouldn't we be checking whether MPIR_FOUND here?

That's a weird CMake syntax thing, not actually an "elseif(another_condition)". See this link:
https://cmake.org/cmake/help/v3.0/command/if.html
where the "if()","else()", and "endif()" all have the same expression in parenthesis.
The expression being copied into the "else()" and endif()" is optional, according to that documentation page, so they can be removed, if you think it is too confusing.

I get that, but I was worried that we set the variables to their MPIR values if GMP is not found, regardless of whether MPIR was found?

Yeah I probably need to fix that. I'm a CMake noob, so any suggestions welcome.

FWIW, hopefully the "find_package_handle_standard_args(MPLIB ..)" below won't set MPLIB_FOUND because MPIR_LIBRARY and MPIRXX_LIBRARY won't be valid/set and hence MPLIB_LIBRARY and MPLIBXX_LIBRARY won't be valid/set ... I hope. :(

marcusmueller · 2018-02-11T15:42:45Z

I'd agree, in this case, doing math right is more important than saving us a few kB of dependency that's actually available on all platforms (albeit under different names). @mbr0wn, new GMP/MPIR in 3.8?

awalls-cx18 · 2018-02-11T17:42:54Z

Two other tag infrastructure fixes I want to do are in this thread:
https://lists.gnu.org/archive/html/discuss-gnuradio/2018-01/msg00042.html
https://lists.gnu.org/archive/html/discuss-gnuradio/2018-01/msg00044.html
and they will build upon this one and use of the MP library.

But maybe these 3 collectively need to be discussed in a GREP, because

I haven't done measurement of the runtime CPU performance impact
For one of the remaining changes, Jeff Long is asking for something that I'm not sure is a good thing to do (leave in a mode were the runtime will still [as a convenience!?] guess about the application of the relative rate and provide approximate, but incorrect, tag propagation).

@mbr0wn

noc0lour · 2018-08-08T00:15:02Z

gnuradio-runtime/lib/block.cc

-    if(relative_rate < 0.0)
-      throw std::invalid_argument("block::set_relative_rate");
+    if(relative_rate <= 0.0)
+      throw std::invalid_argument("block::set_relative_rate: relative rate must be > 0.0");


I like this change!

marcusmueller · 2018-08-17T21:00:30Z

@noc0lour need gnuradio/gnuradio-buildbot#14 for successful building on your Fedora26 worker

marcusmueller · 2018-08-18T16:57:19Z

I'm manually rebasing this (there's really no significant clash here).

noc0lour · 2018-08-18T18:47:07Z

The GMP dep is now available on the build CI.

marcusmueller · 2018-08-19T14:48:46Z

I've noticed! Thank you :)

marcusmueller · 2018-08-23T17:33:20Z

Merged per 75b4ae3. Thank you, @awalls-cx18!!

Andy Walls added 4 commits December 11, 2017 19:25

awalls-cx18 mentioned this pull request Jan 6, 2018

gnuradio-runtime: Optimize some buffer[_reader]_sptr grabs and releases #1537

Closed

runtime: Prefer the GMP library over the MPIR library if both are ava…

d51bfc6

…ilable

runtime: Change 'MPLib' to 'MPLIB' to avoid problems with older CMake…

d4c4e3b

… versions

marcusmueller added this to the Release 3.8 milestone Feb 3, 2018

marcusmueller reviewed Feb 11, 2018

View reviewed changes

marcusmueller added Bug Runtime dependencies labels Feb 11, 2018

marcusmueller requested a review from mbr0wn March 22, 2018 21:10

noc0lour reviewed Aug 8, 2018

View reviewed changes

marcusmueller force-pushed the tag_fix4 branch from 5b6a32e to d4c4e3b Compare August 18, 2018 13:59

marcusmueller closed this Aug 23, 2018

Use multiprecision rational arithmetic to propagate tags #1543

Use multiprecision rational arithmetic to propagate tags #1543

awalls-cx18 commented Jan 5, 2018 •

edited by marcusmueller

noc0lour commented Jan 6, 2018

awalls-cx18 commented Jan 6, 2018 via email

awalls-cx18 commented Jan 6, 2018

awalls-cx18 commented Jan 7, 2018

noc0lour commented Jan 7, 2018

awalls-cx18 commented Jan 7, 2018

noc0lour commented Jan 7, 2018 •

edited

awalls-cx18 commented Jan 8, 2018

marcusmueller commented Feb 3, 2018

awalls-cx18 commented Feb 3, 2018

awalls-cx18 commented Feb 3, 2018

marcusmueller Feb 11, 2018 •

edited

awalls-cx18 Feb 11, 2018

marcusmueller Feb 11, 2018

awalls-cx18 Feb 11, 2018

marcusmueller commented Feb 11, 2018

awalls-cx18 commented Feb 11, 2018

noc0lour Aug 8, 2018

marcusmueller commented Aug 17, 2018

marcusmueller commented Aug 18, 2018

noc0lour commented Aug 18, 2018

marcusmueller commented Aug 19, 2018

marcusmueller commented Aug 23, 2018

Use multiprecision rational arithmetic to propagate tags #1543

Use multiprecision rational arithmetic to propagate tags #1543

Conversation

awalls-cx18 commented Jan 5, 2018 • edited by marcusmueller

noc0lour commented Jan 6, 2018

awalls-cx18 commented Jan 6, 2018 via email

awalls-cx18 commented Jan 6, 2018

awalls-cx18 commented Jan 7, 2018

noc0lour commented Jan 7, 2018

awalls-cx18 commented Jan 7, 2018

noc0lour commented Jan 7, 2018 • edited

awalls-cx18 commented Jan 8, 2018

marcusmueller commented Feb 3, 2018

awalls-cx18 commented Feb 3, 2018

awalls-cx18 commented Feb 3, 2018

marcusmueller Feb 11, 2018 • edited

Choose a reason for hiding this comment

awalls-cx18 Feb 11, 2018

Choose a reason for hiding this comment

marcusmueller Feb 11, 2018

Choose a reason for hiding this comment

awalls-cx18 Feb 11, 2018

Choose a reason for hiding this comment

marcusmueller commented Feb 11, 2018

awalls-cx18 commented Feb 11, 2018

noc0lour Aug 8, 2018

Choose a reason for hiding this comment

marcusmueller commented Aug 17, 2018

marcusmueller commented Aug 18, 2018

noc0lour commented Aug 18, 2018

marcusmueller commented Aug 19, 2018

marcusmueller commented Aug 23, 2018

awalls-cx18 commented Jan 5, 2018 •

edited by marcusmueller

noc0lour commented Jan 7, 2018 •

edited

marcusmueller Feb 11, 2018 •

edited