Dissable threads at compile time with NOTHREADS option

[Chris-F5]

Pull request for #355.
My initial idea was to define empty pthread_* functions in pthreads_cross.{c,h} so that when other code uses them the symbols would get resolved. Then I would enforce worker_pool->nthreads==1 so that these vacuous pthread_* functions didnt get used.
Problem is that these functions need pthread types in their signatures and we cant just typedef our own because its hard to predict if the standard libs will already define them. For example, I'm trying to cross compile to an embedded arm device for which #import <time.h> imports sys/types.h which imports sys/pthread_types.h which defines pthread types even though pthread.h does not declare pthread_* functions!

So the way it works now is if the user defines NOTHREADS then all #include <pthread.h> and all uses of pthread types or functions are dissabled using #ifdefs. Then in workerpool.c we just ignore the nthreads argument to workerpool_create.

Also workerpool.c:workerpool_get_nprocs and common/pthreads_cross.c:pcthread_get_num_procs were re-implementing the same function. So I made workerpool_get_nprocs just call pcthread_get_num_procs. If NOTHREADS is defined then pcthread_get_num_procs returns 1.

I've tested this quickly by running the following code with and without NOTHREADS.

#include <stdio.h>
#include "apriltag.h"
#include "tag25h9.h"
int main(int argc, char *argv[])
{
    image_u8_t* im = image_u8_create_from_pnm("noexist.pnm");
    apriltag_detector_t *td = apriltag_detector_create();
    td->nthreads = 5;
    apriltag_family_t *tf = tag25h9_create();
    apriltag_detector_add_family(td, tf);
    zarray_t *detections = apriltag_detector_detect(td, im);
    for (int i = 0; i < zarray_size(detections); i++) {
        apriltag_detection_t *det;
        zarray_get(detections, i, &det);
        printf("%f %f\n", det->c[0], det->c[1]);
    }
    apriltag_detections_destroy(detections);
    tag25h9_destroy(tf);
    apriltag_detector_destroy(td);
}

Note that these changes have been made on top of my Remove CRLF line ends commit. So diffing the branch makes it look like all the lines in pthreads_cross.{c,h} have changed even though only some have.

[christian-rauch]

The usage of the compile time flag is inconsistent. Either you use a macro to skip compiling phread_cross and the usage of its symbols in the final library, i.e. use it as an optional library, or you use it as an alternative implementation of the pthread functions.

The latter can further be automated by checking which system you are on. If you are on anything UNIX-like that has the pthread.h, you use that as the native implementation of the pthread_ functions. If you are on Windows, you use our custom wrappers. Else, you use dummy noop functions that implement the interface but don't do anything.

I.e. either you do not expose the pthread symbols or you expose them but with a noop implementation.

Also, can you rebase your PR and squash the relevant commits? The summed-up diff changes the entire thread files due to the CRLF changes from the already merged commit.

[Chris-F5]

Yes, I agree the way I was treating pcthread_get_num_procs was inconsistent with the rest of the pthread functions. In this commit, I now use #ifndefs to exclude #include "common/pthreads_cross.h" from the 'outside' so to speak. I could just enclose everything inside pthreads_cross.h with ifndefs but I think that doing it from the outside makes it more explicit whats going on.
Now we also need to skip compiling the functions in pthreads_cross.c as you say. I have done this by enclosing everything in this source file with a big #ifndef NOTHREADS. An alternative solution would be to exclude pthreads_cross.c from compilation entirely in CMakeLists.txt when NOTHREADS CMake option is set. I have only a slight preference for the ifndef solution since it makes us less dependent on the build system.

[christian-rauch]

I think I personally would have preferred to have a noop-pthread implementation to use the same interface throughout the apriltag code to have fewer #ifdef blocks. This then would also have scaled better to other non-UNIX architectures.

How does this now deal with find_package(Threads REQUIRED)? Shouldn't this fail on architectures without threading?

Also, how do you test this? Is this something that can be reproduced on the CI to make sure this compiles and does not break in the future?

[Chris-F5]

The problem with the noop-pthread implementation is how do you deal with the pthread types. If the user has asked for NOTHREADS then we cant assume that their standard libraries will define pthread_t. But if we are to declare noop pthread functions then, since these have pthread types in their signatures, pthread types must be defined. But we cant define these symbols ourselves (a kind of noop type) since we cant assume that their standard libraries do NOT declare pthread_t symbols.

Ultimately, the issue is that we want NOTHREADS to change the meaning of pthread types, when these types are not ours to change the meaning of.

We could choose to get around this by creating our own apriltag_pthread_* types (or something the like). On normal unix systems these would be typedef'd to their pthread_ equivalents. On Windows or other non-unix architectures they could be typedefed to whatever the systems native is. If NOTHREADS is enabled then they could by typedefed to a void pointer or something. By doing this, when we are on non-unix systems or when NOTHREADS is enabled, we wouldnt have to worry about weather or not pthread_* types are already defined. Then we could also get rid of the #ifndef NOTHREADS outside of common/pthreads_cross.{c,h}.

IMO the apriltag_pthread_* type idea is only worth the complexity (and possible confusion of having our own pthread types) if we are worried about scaling to other non-unix architectures in the future. I'd be happy to try and implement the apriltag_pthread idea if this is the case.

I have been testing these changes by compiling with and without the NOTHREADS cmake option, then running the simple detection code (in original PR comment) and inspecting the output of strace to verify that threads are and are not being created. I have also been using these changes in my micropython port, in which I cross compile the source (using a separate build system) to a system without pthread_ functions.

If we wanted to make automated tests for this, I think it would be sufficient to check that cmake -B build -DNOTHREADS=ON does not encounter any compilation errors.

[christian-rauch]

We could choose to get around this by creating our own apriltag_pthread_* types (or something the like). On normal unix systems these would be typedef'd to their pthread_ equivalents. On Windows or other non-unix architectures they could be typedefed to whatever the systems native is. If NOTHREADS is enabled then they could by typedefed to a void pointer or something. By doing this, when we are on non-unix systems or when NOTHREADS is enabled, we wouldnt have to worry about weather or not pthread_* types are already defined. Then we could also get rid of the #ifndef NOTHREADS outside of common/pthreads_cross.{c,h}.

You do not need to typedef an extra type. You just need to map everything to the UNIX type to replicate that API. Have a look at https://github.com/AprilRobotics/apriltag/blob/master/common/pthreads_cross.h to see how the pthread_ types are typedefed on Windows. You can do the same for a non-threaded dummy. By implementing the same threading interface that is used on UNIX, we do not have to add specific behaviour outside of where the functions are used.

On top of that, we can just test with macros on which platform we are on and define the types and functions appropriately. There is no need for a special flag, unless we also want to provide that non-threading option on UNIX and Windows.

[Chris-F5]

Here is my best attempt at guessing weather pthread_* types and or symbols are already defined by looking at Posix macros. Not sure if this is what you had in mind. I have left find_package(Threads REQUIRE) in the CMake for this commit since its hard for me to test this implementation without access to a machine which doesnot support threads but is capable of running cmake.
I must admit I prefer the NOTHERADS macro to explicitly dissable threading. Here we are relying on __POSIX_THREAD and __POSIX_VISIBLE to guess if pthread symbols are already defined. Say a user is on a system withhout _POSIX_THREADS, building a project which links with apriltag and some other library. If this other library also decides to declare its own noop pthread_ symbols then we will get a symbol collision. The NOTHREADS implementation is also easier to test since we can freely enable or disable NOTHREADS without other side effects.

[christian-rauch]

With the three options (win32, pthread, no pthread) I would restructure this differently.

In the header, if _POSIX_THREADS is defined, reuse the definitions from the pthread.h. Else, re-define types and functions. That's it. I don't think we have to check for _WIN32 in the header, unless we want to define Windows-specific types.

For the implementation in the source, I would also first check if _POSIX_THREADS is defined, is so "do nothing". else-if _WIN32 defined, add the Windows wrapper, else (no pthread, no windows) add the noop implementations.

IMHO, we shouldn't need changes in any other files, such as the workerpool.

[christian-rauch]

I find the Not _WIN32. comment misleading, since this is an elif. So this section is not only defined when _WIN32 is not defined, but also it needs _POSIX_THREADS to not be defined.

[christian-rauch]

Also here, I find the comment _WIN32 not defined to be misleading.

[christian-rauch]

Isn't this duplicating the WIN32 section above?

[christian-rauch]

Where is NOTHREADS defined? Shouldn't this be conditioned on _POSIX_THREADS too?

[christian-rauch]

@Chris-F5 Are you still interested in this PR?

[Chris-F5]

Not really. If nobody else is interested we can close.

[christian-rauch]

I think having the option to compile the library without threading support is still useful. If this gets implemented, there should be one common API definition in the header, probably replicating the POSIX thread API, and the different implementations, including noop, in the source file.

But if someone is interested in implementing this, it also needs a check in the CI to make sure that this actually compiles and works in an embedded setting without threading support.

I am going to close this now. Anyone interested in this can pickup from here.