RFC: Fl::enable_locks()

[Greg Ercolano]

The more I look at it, the more this looks like a bug in someone's applcation code:

int main() {
    ..
    Fl::lock();   // lock without an unlock() tells fltk to enable locking
    return Fl::run();
}

While that's apparently the "right way" to inform FLTK the app is going to use locking, it looks wrong and feels wrong for app code to have a lock() without an unlock().

I'd like to suggest we make a simple wrapper function called:

    Fl::enable_locks();

Underneath that might just call Fl::lock(), but at least then the user's app code looks clearer:

int main() {
    ..
    Fl::enable_locks();
    return Fl::run();
}

There's no chance of someone getting confused by this code.

If we do this, our docs can say that new code should use this to enable locking,
and mention the 'old way' was to call Fl::lock() without a corresponding unlock
before the app loop to enable locking.

I've actually never used Fl::lock() before, so perhaps it's just because I'm new to this
it'll somehow grow on me or something, but on first encountering, it just seems
really unsymmetrical and lacks clarity.

Comments?

[Ian MacArthur]

On 17 Apr 2021, at 07:01, Greg Ercolano wrote:
>
> The more I look at it, the more this looks like a bug in someone's applcation code:
> int main() {
> ..
> Fl::lock(); // lock without an unlock() tells fltk to enable locking
> return Fl::run();
> }
>
> While that's apparently the "right way" to inform FLTK the app is going to use locking, it looks wrong and feels wrong for app code to have a lock() without an unlock().
>
> I'd like to suggest we make a simple wrapper function called:
>
> Fl::enable_locks();

I’m “used” to the way we do it at present, so it no longer looks weird.

However, if we want to add a wrapper, as Greg suggests, I’d like it to have a name that *somehow* tells the user it needs to be done only in the main thread, and only the one time.

That said, I do not have a good name. Maybe enable_locks() is a good name.

Maybe something explicit like Fl::initialize_thread_support() or such - it’s long, but at least it reflects what it is actually doing!

[Manolo]

On Saturday, April 17, 2021 at 8:01:54 AM UTC+2 er...@seriss.com wrote:

The more I look at it, the more this looks like a bug in someone's applcation code:

int main() {
    ..
    Fl::lock();   // lock without an unlock() tells fltk to enable locking
    return Fl::run();
}

While that's apparently the "right way" to inform FLTK the app is going to use locking, it looks wrong and feels wrong for app code to have a lock() without an unlock().

I'd like to suggest we make a simple wrapper function called:

    Fl::enable_locks();

Underneath that might just call Fl::lock(), but at least then the user's app code looks clearer:

int main() {
    ..
    Fl::enable_locks();
    return Fl::run();
}

Comments?

As Ian, I'm used to this mechanism and don't find it disturbing.

It's well documented in the part of the manual devoted to multithreading, and also

in the doc of Fl::lock().

The economy of means of having Fl::lock() do different things is nice, but a new name

might be clearer for new developments.

Thus, I'm neither against nor in favor of adding a new, well chosen, function name.

[Albrecht Schlosser]

On 4/17/21 8:01 AM Greg Ercolano wrote:
> The more I look at it, the more this looks like a bug in someone's
> applcation code:
>
> int main() {
>     ..
>     Fl::lock();   // lock without an unlock() tells fltk to enable
> locking
>     return Fl::run();
> }
>
>
> While that's apparently the "right way" to inform FLTK the app is going
> to use locking, it looks wrong and feels wrong for app code to have a
> lock() without an unlock().

Well, there's no problem, you can also write

int main() {
..
Fl::lock(); // lock FLTK (this) thread
int ret = Fl::run();
Fl::unlock(); // unlock
return ret;
}

;-)

Honestly, I'm not sure if Fl::run() would return with the lock still
held or unlocked. That's just a note.

> I'd like to suggest we make a simple wrapper function called:
>

> *Fl::enable_locks();*

>
> Underneath that might just call Fl::lock(), but at least then the user's
> app code looks clearer:
>
> int main() {
>     ..
>     Fl::enable_locks();
>     return Fl::run();
> }
>
> There's no chance of someone getting confused by this code.
>

> Comments?

As others have said already, I also wouldn't mind adding such a
function. Other method names might be:

Fl::start_lock();
Fl::use_lock();

An alternative and IMHO clean way to clarify things would be to add an
optional argument to Fl::run():

static int Fl::run(int enable_locks = -1) {...}

The default (-1) would behave as before (compatibility mode) and require
Fl::lock() before Fl::run() is called or work w/o locks.

Value 0 would not call lock/unlock internally - as before if the
application doesn't call Fl::lock().

Value 1 would call Fl::lock() right at the beginning of Fl::run() and
thus replace the single Fl::lock() call before Fl::run(). We could even
check if Fl::lock() has been called before to avoid double locking, but
that should be in the users responsibility.

The more I think about it, the more I like the latter, i.e. adding such
a parameter to Fl::run().

And FLTK 1.4 would be a good place to start this...

[Michael Sweet]

Another crazy option: just have Fl::lock() be a no-op if the run loop is not active and then have Fl::run() call Fl::lock() unconditionally. When threading is enabled (which should be all the time now) the right thing happens without any special initialization, and Fl::lock()/unlock() can continue to be used for synchronization as before.

The new (1.4) code examples can explain this change and existing code will continue to work.

> --
> You received this message because you are subscribed to the Google Groups "fltk.coredev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to fltkcoredev...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/fltkcoredev/d252cdf7-08ba-9562-69fe-095640617087%40online.de.

________________________
Michael Sweet

[Greg Ercolano]

On 4/17/21 4:36 AM, Albrecht Schlosser wrote:

On 4/17/21 8:01 AM Greg Ercolano wrote:

While that's apparently the "right way" to inform FLTK the app is going to use locking, it looks wrong and feels wrong for app code to have a lock() without an unlock().

Well, there's no problem, you can also write

  int main() {
    ..
    Fl::lock();   // lock FLTK (this) thread
    int ret = Fl::run();
    Fl::unlock(); // unlock
    return ret;
  }

;-)

    LOL, that just looks worse -- it looks like we're locking out child threads from the entire app loop.

As others have said already, I also wouldn't mind adding such a function. Other method names might be:

  Fl::start_lock();
  Fl::use_lock();

An alternative and IMHO clean way to clarify things would be to add an optional argument to Fl::run():

  static int Fl::run(int enable_locks = -1) {...}

    Oh, that's an interesting idea.

    But hmm, what if there's other features we need to enable in the future? Add them as arguments too?

    Might be better to be a separate method with the name 'lock' in it, so it can appear
    in the threading docs along with the other lock related methods.

    Another option might be to use Fl::option(ENABLE_LOCKING);

The more I think about it, the more I like the latter, i.e. adding such a parameter to Fl::run().
And FLTK 1.4 would be a good place to start this...

    I like the idea if C++ supported named arguments like in python, e.g.

        Fl::run(enable_locks = 1);

    But to just have:

        Fl::run(1);

    ..seems unclear offhand. I'm trying to propose something so the code reads clearly
    what it does without comments.

[Albrecht Schlosser]

On 4/17/21 6:03 PM Greg Ercolano wrote:
>
> On 4/17/21 4:36 AM, Albrecht Schlosser wrote:
>>
>> Well, there's no problem, you can also write
>>
>>   int main() {
>>     ..
>>     Fl::lock();   // lock FLTK (this) thread
>>     int ret = Fl::run();
>>     Fl::unlock(); // unlock
>>     return ret;
>>   }
>>
>> ;-)
>
>     LOL, that just looks worse -- it looks like we're locking out child
> threads from the entire app loop.

Yes, and that's what it is doing. See it this way: before you start the
FLTK event loop with Fl::run() you set a FLTK lock in the main thread.
You could then start other threads that are locked out from FLTK access
and would block if they call Fl::lock(). You could also create widgets
and windows here (after locking FLTK) in the main thread before you
execute Fl::run().

Then the FLTK event loop /releases/ the lock /temporarily/ while it has
nothing to do (during Fl::wait()) so other threads waiting for the lock
can access FLTK objects. After Fl::wait() the main FLTK thread acquires
the lock again, does its job, and finally releases it before it waits again.

Calling Fl::lock() in the main thread is NOT different than calling it
in any other place (as you say: an "overload"). It only has the side
effect that the /first/ execution of Fl::lock() enables locking
internally (no matter when or where you call it). At least that's what I
believe.

Calling Fl::unlock() after Fl::run() is basically useless (but could be
done) because the program exits anyway. But there could be other
scenarios...

>> An alternative and IMHO clean way to clarify things would be to add an
>> optional argument to Fl::run():
>>
>>   static int Fl::run(int enable_locks = -1) {...}
>
>     Oh, that's an interesting idea.
>
>     But hmm, what if there's other features we need to enable in the
> future? Add them as arguments too?

Have we ever needed an extra feature in Fl::run() in the last 22 years? ;-)

>     Might be better to be a separate method with the name 'lock' in it,
> so it can appear
>     in the threading docs along with the other lock related methods.

Just RTFM: Fl::lock() and Fl::unlock() ;-)

>     Another option might be to use Fl::option(ENABLE_LOCKING);

-1

Fl::option() is "overloaded" with too many features and an ugly
interface (IMHO). It does also use preferences which would be bad here.
(Does it? I'm not sure.)

>> The more I think about it, the more I like the latter, i.e. adding
>> such a parameter to Fl::run().
>> And FLTK 1.4 would be a good place to start this...
>
>     I like the idea if C++ supported named arguments like in python, e.g.
>
>         Fl::run(enable_locks = 1);
>
>     But to just have:
>
>         Fl::run(1);
>
>     ..seems unclear offhand. I'm trying to propose something so the
> code reads clearly what it does without comments.

Honestly? Sorry, I disagree.

<joke>
#define RUN_WITH_LOCKS 1
Fl::run(RUN_WITH_LOCKS);
</joke>

Seriously: there are lots of functions with one or more parameters that
might not be absolutely clear w/o accessing the documentation. If
Fl::run(int use_lock) (or 'enable_locks' as I suggested before) is
well-documented, then there should be no doubt.

Here are just two examples of methods with "unclear" parameters
(actually three because the first uses an overload):

void Fl::add_fd(int fd, int when, Fl_FD_Handler cb, void *d = 0);
void Fl::add_fd(int fd, Fl_FD_Handler cb, void *d = 0);
https://www.fltk.org/doc-1.4/classFl.html#a8905e303e45e846b8ce77168f4cf7afe

Fl_Image::scale(int width, int height, int proportional=1, int
can_expand=0);
https://www.fltk.org/doc-1.4/classFl__Image.html#a3aa152a98c5a8eac1012d7d115324d18

You'll see code like

image->scale(w, h, 0, 1);

I just picked these as /examples/. There are lots of other similar cases.

Please don't understand me wrong, I'm open for all kinds of
implementations (except maybe Fl::option()) but it was just that I could
not follow your argumentation.

[Albrecht Schlosser]

On 4/17/21 2:25 PM Michael Sweet wrote:

> Another crazy option: just have Fl::lock() be a no-op if the run loop is not active and then have Fl::run() call Fl::lock() unconditionally. When threading is enabled (which should be all the time now) the right thing happens without any special initialization, and Fl::lock()/unlock() can continue to be used for synchronization as before.

Mike, IIRC there's still the "side effect" that the first call into
Fl::lock() enables the locking and unlocking feature. Hence, if you
never call it, locking and unlocking will not be done by the FLTK thread
(event loop), which is probably one of "fast and light" design aspects
of FLTK.

Hence, calling "Fl::lock() unconditionally" would add the lock/unlock
"overhead" (even if it's minimal) in apps that don't need it.

Please correct me if I'm wrong.

[Bill Spitzak]

It is intended that Fl::lock() be called, it is not just enabling locking, you are in fact locking it. The actual hack is that fltk does not require this, for back compatibility and so a single-threaded program does not need to call the lock.

An alternative would be to do a fatal assert if lock was not called but that would be annoying for single-threaded programs.

Starting out "already locked" makes it difficult to make procedures that can be used by different threads and want to unlock fltk.

--
You received this message because you are subscribed to the Google Groups "fltk.coredev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkcoredev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/fltkcoredev/c315ce11-cec6-88cd-49d4-efdbd524546a%40online.de.

[Greg Ercolano]

On 4/17/21 9:46 AM, Albrecht Schlosser wrote:

On 4/17/21 6:03 PM Greg Ercolano wrote:

On 4/17/21 4:36 AM, Albrecht Schlosser wrote:

Well, there's no problem, you can also write

  int main() {
    ..
    Fl::lock();   // lock FLTK (this) thread
    int ret = Fl::run();
    Fl::unlock(); // unlock
    return ret;
  }

;-)

     LOL, that just looks worse -- it looks like we're locking out child threads from the entire app loop.

Yes, and that's what it is doing.

    Yes, but what's not clear is that FLTK turns it off..
    because the app code turned it on.

    I just wouldn't expect FLTK's internals to release my Fl::lock() call.. ever.

    But I'm probably missing some flexibility that using Fl::lock() this way allows.

    I mean, I expect FLTK's internals to manage the same locking mutex
    to engage and disengage the lock for synchronization with the child threads.

    It's just that when I (the app) turns on Fl::lock(), I expect it to remain on until
    /I/ turn it off.

    For me, the internals of FLTK's lock management should be invisible to me,
    which is why I think it's clearer to say:

        Fl::enable_locks();

    ..because it would imply enabling FLTK's internals to begin lock management.

    It's just I really strain for me to see past FLTK undoing my Fl::lock() call for me,
    ever. As a design concept I mean.

See it this way: before you start the FLTK event loop with Fl::run() you set a FLTK lock in the main thread. You could then start other threads that are locked out from FLTK access and would block if they call Fl::lock().

You could also create widgets and windows here (after locking FLTK) in the main thread before you execute Fl::run().

    Right, and I need that for sure because I'm starting child threads
    before the Fl::run() call (and after the Fl::lock(), apparently)

    Since one would want to create threads before Fl::run(), isn't that a reason
    to not to put the lock flag control in the Fl::run() call?

Then the FLTK event loop /releases/ the lock /temporarily/ while it has nothing to do (during Fl::wait()) so other threads waiting for the lock can access FLTK objects. After Fl::wait() the main FLTK thread acquires the lock again, does its job, and finally releases it before it waits again.

    Right, I do get all that -- I do.

Calling Fl::lock() in the main thread is NOT different than calling it in any other place (as you say: an "overload").

    What I meant by 'overload', is when called from the main thread before Fl::run(),
    it's serving a dual purpose: is it's /enabling/ FLTK's internal use of locks.. because
    without the call, it doesn't use them.

    So when used in the main thread, Fl::lock() serves not only the purpose of locking
    my child threads, but also changes the behavior of FLTK to use locking.
    A "mode setting", if I understand it correctly. That's what I find "unexpected".

    I think most folks would expect FLTK initializes itself with locks engaged
    (there may be reasons not to, which is why it doesn't?), and one wouldn't
    even need to turn them on from the main thread, FLTK's internals would
    initialize them locked (so child threads could be created safely before Fl::run)
    and the app loop would control them automatically internally.

    I expect there's history involved here, where FLTK's development predates
    the standardized use of threads. But I imagine new libraries created today
    implement locking automatically to be thread safe by default, and not an option.

It only has the side effect that the /first/ execution of Fl::lock() enables locking internally (no matter when or where you call it). At least that's what I believe.

    Right -- it's that 'side effect' that makes it dual-purpose to me, or 'overloaded'.

    Fl::lock() before run() does more than just engage a lock until I clear it,
    it enables locking in FLTK, and FLTK will unlock it for you when it needs to.
    That I also find unexpected -- that it'll clear the lock I made.

    It just makes me feel weird about Fl::lock(): I want to think of it as a simple
    thing that does one thing only, lock until I clear it.

    The fact FLTK clears it if I call it before Fl::run() makes it 'not simple'.

     Another option might be to use Fl::option(ENABLE_LOCKING);

-1

Fl::option() is "overloaded" with too many features and an ugly interface (IMHO). It does also use preferences which would be bad here. (Does it? I'm not sure.)

    Oh, well I'd agree on -1 if that were the case.

    But Fl::option() seems to be for turning FLTK global features on and off,
    like OPTION_VISIBLE_FOCUS, OPTION_SHOW_TOOLTIPS, etc.

    But I'd withdraw that suggestion; a dedicated method call is better.

Please don't understand me wrong, I'm open for all kinds of implementations (except maybe Fl::option()) but it was just that I could not follow your argumentation.

    I just wouldn't expect the control of locking to be an argument to Fl::run().

    See above as to why it's probably better to be implemented as a method
    so the app can turn it on so threads created before Fl::run() can be protected.

[Michael Sweet]

Albrecht,

On Apr 17, 2021, at 1:00 PM, Albrecht Schlosser <Albrech...@online.de> wrote:

On 4/17/21 2:25 PM Michael Sweet wrote:

Another crazy option: just have Fl::lock() be a no-op if the run loop is not active and then have Fl::run() call Fl::lock() unconditionally. When threading is enabled (which should be all the time now) the right thing happens without any special initialization, and Fl::lock()/unlock() can continue to be used for synchronization as before.

Mike, IIRC there's still the "side effect" that the first call into Fl::lock() enables the locking and unlocking feature. Hence, if you never call it, locking and unlocking will not be done by the FLTK thread (event loop), which is probably one of "fast and light" design aspects of FLTK.

Calling lock/unlock methods in the event loop, even for single-threaded applications, isn't going to add any detectable change in performance.  Consider the following simple benchmark program that locks and unlocks a mutex one billion times:

#include <stdio.h>

#include <time.h>

#include <sys/time.h>

#include <pthread.h>

int main(void)

{

  struct timeval start, end;

  int i;

  pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;

  double secs;

  gettimeofday(&start, NULL);

  for (i = 0; i < 1000000000; i ++)

  {

    pthread_mutex_lock(&mutex);

    pthread_mutex_unlock(&mutex);

  }

  gettimeofday(&end, NULL);

  secs = end.tv_sec - start.tv_sec + 0.000001 * (end.tv_usec - start.tv_usec);

  printf("%.3f seconds\n", secs);

  return (0);

}

On my new M1 MacBook Pro, it took just 8.659 seconds to complete, which means that each pair of lock/unlock took 8.659 *picoseconds*.  On my older 18-core iMac Pro it took a little over twice the time - 18.036 seconds - which works out to 18 picoseconds per pair.  On a Raspberry Pi 0 (probably the slowest of my computing devices) it took 155.639 seconds or about 155 picoseconds per lock/unlock pair.

So honestly, the overhead of using threading locks, even for a single-threaded program, is so small that I would recommend that we make FLTK thread-safe by default.

________________________
Michael Sweet

[Michael Sweet]

Bill,

> On Apr 17, 2021, at 2:08 PM, Bill Spitzak <spi...@gmail.com> wrote:
> ...

> Starting out "already locked" makes it difficult to make procedures that can be used by different threads and want to unlock fltk.

Can you provide an example? Given the example for using threading:

Fl::lock();
Fl::run();

Fl::run() starts with things already locked...

________________________
Michael Sweet

[Bill Spitzak]

The most important unlock is when the main thread is not doing anything and waiting for events. This is done internally by fltk. After an event comes in it re-locks before doing anything with the event. You absolutely do not want the lock held all the time it waits for an event, that would defeat the entire purpose.

--
You received this message because you are subscribed to the Google Groups "fltk.coredev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkcoredev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/fltkcoredev/649B330D-C92B-441B-B9A0-E8E46EF8260A%40gmail.com.

[Greg Ercolano]

On 4/18/21 6:48 AM, Michael Sweet wrote:

So honestly, the overhead of using threading locks, even for a single-threaded program, is so small that I would recommend that we make FLTK thread-safe by default.

    Right, that'd be great if possible.

    We just have to make sure that if FLTK starts with locks enabled, old apps calling Fl::lock()
    before run() won't hang on the lock call.

[Michael Sweet]

Greg,

My thought was that Fl::lock() and Fl::unlock() could be no-ops until Fl::run() is called.

________________________
Michael Sweet

[Ian MacArthur]

On 18 Apr 2021, at 14:48, Michael Sweet wrote:
>
> Calling lock/unlock methods in the event loop, even for single-threaded applications, isn't going to add any detectable change in performance. Consider the following simple benchmark program that locks and unlocks a mutex one billion times:
>

Hmmm... We might need to proceed with some caution here, though, as those timing metrics are very sensitive to hitting the mutex and the pthread functions in cache and such (which Mike’s program of course does.)

As an alternate (for those with 64-bit intel machines and a gcc-like compiler on a posix-like host, anyway) I offer the attached, which yields the following numbers:

Lock time : 43612 (1 iterations)
Unlock time : 23426 (1 iterations)
Lock time : 48 (10 iterations)
Unlock time : 44 (10 iterations)
Lock time : 122 (100 iterations)
Unlock time : 43 (100 iterations)

This was captured on an older iMac, with 2.9Ghz nominal clock speed.
The counts and basically in machine cycles, so that is saying a single, out-of-cache lock operation takes ~43000 cycles (15us) and the single unlock takes ~23000 cycles (8us).
Once the mutex and the lock/unlock code is cached, the times drop off to be a wee bit more like Mike’s numbers; but if you run the code a few times yourself, you can “see" the runs where the mutex calls miss the cache sometimes and the numbers jump up... 50 cycles is ~16ns, and 120 cycles is ~42ns.

If we are “busy” then the mutex (and probably the pthread functions) will be bumped out of cache so there’s a fair chance the timings might look more like the 15us (or 8us) values, rather than the 16ns values...

[Greg Ercolano]

    Sounds worth trying.

    Perhaps we can test this by sticking this into 1.4.x and see if anything shakes out from dev testing.

    The safe thing to do would be to make a new configure/cmake flag for it (default on)
    that can be turned off to disable the new behavior if it causes trouble.
   

[Bill Spitzak]

You can't get rid of the need for the main thread to call lock() before it calls any fltk calls by having it locked already. If you implement recursive locks then you will be unable to reliably unlock before wait. If the locks are not recursive you will deadlock. You would have to make different versions of every function making fltk calls, one which locks and one which does not, depending on which thread it is called from.

Making the internal unlock+lock a no-op until the program calls lock() is I think exactly what it is doing now.

--

You received this message because you are subscribed to the Google Groups "fltk.coredev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to fltkcoredev...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/fltkcoredev/13b64cf2-659e-c0ad-5d15-908194b80f88%40seriss.com.

[Ian MacArthur]

On 18 Apr 2021, at 14:48, Michael Sweet wrote:
>

> Calling lock/unlock methods in the event loop, even for single-threaded applications, isn't going to add any detectable change in performance.

Re-reading this thread, it occurs to me that we do need to be careful here because (timing of mutex operations aside) many (most? all?) OS implementations will treat acquiring or releasing of a lock as a scheduling point, potentially entailing a (hopefully very) brief trip through the scheduler, that may result in the thread being suspended in favour of some other thread.
(The scheduling points arise because the acquire operation may block, and the release operation may make some higher priority thread runnable.)

So even in a single-thread application, the addition of these extra potential scheduling points may alter the flow of the application in unforeseen ways...

It’s tricky.

[Albrecht Schlosser]

On 4/18/21 9:08 PM Greg Ercolano wrote:
> On 4/18/21 11:10 AM, Michael Sweet wrote:
>
>>> On Apr 18, 2021, at 1:33 PM, Greg Ercolano<er...@seriss.com> wrote:
>>>
>>> Right, that'd be great if possible.
>>>
>>> We just have to make sure that if FLTK starts with locks enabled, old apps calling Fl::lock()
>>> before run() won't hang on the lock call.
>> My thought was that Fl::lock() and Fl::unlock() could be no-ops until Fl::run() is called.
>
>     Sounds worth trying.

I don't think that this would be a good idea. See notes below.

>     Perhaps we can test this by sticking this into 1.4.x and see if
> anything shakes out from dev testing.
>
>     The safe thing to do would be to make a new configure/cmake flag
> for it (default on)
>     that can be turned off to disable the new behavior if it causes
> trouble.

Whatever we do, using configure or CMake flags should IMHO not change
such basic behavior. Think of Linux and other distros providing FLTK.
What should they do, which options should they choose? If there are too
many options this would likely force more and more users to build their
own versions of FLTK.

Going back to the original problem: after thinking about it (and looking
at the code/comments) it became clear to me that the user must be able
to call Fl::lock() before calling Fl::run() and that the call before
Fl::run() must not be a no-op.

Reasoning:

(1) First a code comment, likely from Bill, in src/Fl_lock.cxx:

```
The API:

Fl::lock() - recursive lock. You must call this before the
first call to Fl::wait()/run() to initialize the thread
system. The lock is locked all the time except when
Fl::wait() is waiting for events.
```

So, what does this say? First of all, Fl::lock() is a recursive lock.
You must call it /exactly/ once /before/ Fl::run() is called. Let aside
the phrase "to initialize the thread system". This is only relevant to
distinguish multithreaded and singlethreaded FLTK applications.

We should update the documentation accordingly and clarify this fact.


(2) Why do we need to use an operational (not a no-op) call of
Fl::lock() /before/ calling Fl::run()?

It's about initialization of threads that need access to FLTK data and
call Fl::lock() / Fl::unlock() later. The attached file
fltk_test_threads_cxx.txt is an excerpt from our test/threads.cxx:
shortened but with additional comments marked "// # ".

Please read the comments carefully.

Although there might be other solutions (see FLTK docs about "lockless
operation" in the "Advanced FLTK" chapter) this is a valid FLTK program
that should make clear (in general) why the Fl::lock() call before
Fl::run() must not be a no-op.

Please don't say "it doesn't matter if the child threads are locked or
not". There may be cases in user programs where it DOES matter.


(3) I withdraw my proposal to add a parameter to Fl::run() to start
locking with an initial Fl::lock() inside Fl::run() because there are
valid scenarios (see 2 above) that need to call Fl::lock() before
calling Fl::run(). This would only make it more complicated than necessary.

Note that Fl::lock() must not (accidentally) be called twice because it
is a recursive lock.

After all I believe the current behavior

(a) is correct and
(b) can't be improved much.

If we want to support single-threaded programs w/o the requirement to
call Fl::lock() [and I think we do, see test/hello.cxx] we should not
enforce calling Fl::lock() in any way.

I propose however to clarify the docs and particularly mention the
difference of running FLTK single-threaded (w/o Fl::lock()) vs
multi-threaded (with Fl::lock()).

That said, I also believe that adding another method like
Fl::enable_locks() is not helpful and would make the entire stuff even
more confusing.

Ian provided some interesting aspects why we may still want to keep the
ability to run FLTK in single-threaded mode. Thanks.

[Albrecht Schlosser]

On 4/18/21 7:08 AM Greg Ercolano wrote:
> On 4/17/21 9:46 AM, Albrecht Schlosser wrote:

>> It only has the side effect that the /first/ execution of Fl::lock()
>> enables locking internally (no matter when or where you call it). At
>> least that's what I believe.
>
>     Right -- it's that 'side effect' that makes it dual-purpose to me,
> or 'overloaded'.

Greg, I wrote in another message some minutes ago why I believe that the
current behavior is necessary and correct, so I'm replying here only on
this single aspect.

You wrote:

>     Fl::lock() ... enables locking in FLTK, and FLTK will _unlock it for you_ when

> it needs to.
>     That I also find unexpected -- that it'll clear the lock I made.

I don't understand the differentiation between "I" (you, the programmer)
and "it" (FLTK in this context).

We're talking about the main thread that eventually executes Fl::run()
and other (child) threads that want to access FLTK structures and need
to call Fl::lock().

For me there's no difference between "my" thread and "FLTK". There's
just one rule:

| If I want to use threads and Fl::lock() I |
| *must* lock the main (FLTK) thread before |
| I execute Fl::run(). |

In other words: use Fl::lock() or not, but if you use it, lock the main
(FLTK) thread by calling Fl::lock() before calling Fl::run(). That's it.

After that the main thread - not an abstract "FLTK" instance - owns the
lock and releases it temporarily as appropriate when it doesn't need it
while waiting for events, thus enabling other (child) threads to access
FLTK structures, widgets, and such.

This main FLTK thread is still my ("your") thread and works as it is
designed.

After all I'm -1 on adding just another call to "enable" FLTK locking.
What if anybody calls 'Fl::enable_locks(0);' and later 'Fl::lock();'?
Should Fl::lock() then be honored or ignored? As I said in my other
reply, this would IMHO make it only more confusing.

Docs should be clarified though.

[Ian MacArthur]

On 18 Apr 2021, at 19:35, Ian MacArthur wrote:
>
> As an alternate (for those with 64-bit intel machines and a gcc-like compiler on a posix-like host, anyway) I offer the attached, which yields the following numbers:
>
> Lock time : 43612 (1 iterations)
> Unlock time : 23426 (1 iterations)
> Lock time : 48 (10 iterations)
> Unlock time : 44 (10 iterations)
> Lock time : 122 (100 iterations)
> Unlock time : 43 (100 iterations)

Here’s a revised version tweaked to build for either Posix or Win32 hosts (still assuming 64-bit intel and gcc tools though.)
I also tweaked it to take the nominal CPU clock speed (in GHz) as a command line parameter...

Running on Win10 on a, nominally, 2.21GHz CPU, I see:

$ ./mutex_test.exe 2.21
Lock time : 3152 (1 iterations) -- 1426.24ns
Unlock time : 2542 (1 iterations) -- 1150.23ns
Lock time : 1859 (10 iterations) -- 841.18ns
Unlock time : 5598 (10 iterations) -- 2533.03ns
Lock time : 1582 (100 iterations) -- 715.84ns
Unlock time : 1526 (100 iterations) -- 690.50ns

$ ./mutex_test.exe 2.21
Lock time : 2534 (1 iterations) -- 1146.61ns
Unlock time : 2058 (1 iterations) -- 931.22ns
Lock time : 1606 (10 iterations) -- 726.70ns
Unlock time : 1527 (10 iterations) -- 690.95ns
Lock time : 1580 (100 iterations) -- 714.93ns
Unlock time : 1528 (100 iterations) -- 691.40ns


Which suggests that the WIN32 mutex is a good bit slower than the MacOS mutex I was testing before.
(Which, actually, I think I knew, now I think about it...)

This is just for info., really.

Code attached...

[Ian MacArthur]

On 19 Apr 2021, at 15:14, Albrecht Schlosser wrote:
>
>
> What if anybody calls 'Fl::enable_locks(0);' and later 'Fl::lock();'? Should Fl::lock() then be honored or ignored? As I said in my other reply, this would IMHO make it only more confusing.
>

If the Fl::enable_locks(0); call takes the lock, and the Fl::lock(); also takes the lock, and both occur in main(), say, then given that it is a recursive lock, the “main” thread will be able to lock twice OK - you can always (recursively) take a lock you already hold.
I do not think there is any way for the fltk-core to determine if the “extra” call to Fl::lock(); is actually extra or not at runtime...

But the recursive count, presumably, will not go to zero when the fltk-core unlocks(), so no child thread will be allowed access. This is unlikely to be what was intended...

[Albrecht Schlosser]

On 4/19/21 5:01 PM Ian MacArthur wrote:
> On 19 Apr 2021, at 15:14, Albrecht Schlosser wrote:
>>
>>
>> What if anybody calls 'Fl::enable_locks(0);' and later 'Fl::lock();'? Should Fl::lock() then be honored or ignored? As I said in my other reply, this would IMHO make it only more confusing.
>
> If the Fl::enable_locks(0); call takes the lock, and the Fl::lock(); also takes the lock, and both occur in main(), say, then given that it is a recursive lock, the “main” thread will be able to lock twice OK - you can always (recursively) take a lock you already hold.

I intentionally wrote 'Fl::enable_locks(0);' which would (as proposed)
/not/ enable locking. This was intended to point out a contradiction if
Fl::lock() was called later.

But if (e.g.) 'Fl::enable_locks(1);' (note: argument 1) takes the lock
and Fl::lock(); also takes the lock ...

> I do not think there is any way for the fltk-core to determine if the “extra” call to Fl::lock(); is actually extra or not at runtime...

I agree. We could count the locks but we could not determine if two
(nested) locks are intended or not.

> But the recursive count, presumably, will not go to zero when the fltk-core unlocks(), so no child thread will be allowed access. This is unlikely to be what was intended...

FTR, just for my own curiosity I tested this by adding a second
Fl::lock() call in main() of test/threads.cxx and, as expected, the two
windows showed up but there was no update of the results (prime
numbers). Note: using Linux, if that matters (it shouldn't).

[Albrecht Schlosser]

On 4/17/21 2:25 PM Michael Sweet wrote:

> Another crazy option: just have Fl::lock() be a no-op if the run loop is not active and then have Fl::run() call Fl::lock() unconditionally. When threading is enabled (which should be all the time now) the right thing happens without any special initialization, and Fl::lock()/unlock() can continue to be used for synchronization as before.
>
> The new (1.4) code examples can explain this change and existing code will continue to work.

Mike and others,

I thought again about your suggestion. Although I'm sure that we must
not make Fl::lock() and Fl::unlock() no-ops if the run loop is not
active this model could still work *if* we wanted to enable locking
*unconditionally* in all FLTK applications.

(1) Fl::lock() and Fl::unlock() must always be functional, even before
Fl::run() is called.

(2) We increment and decrement our own internal lock counts in
Fl::lock() and Fl::unlock(), resp.

(3) At the entry of Fl::run() we check if Fl::lock() is effective
(internal lock count > 0) and if not, call Fl::lock() internally only
once when Fl::run() starts the loop.

(4) The internal unlock/wait/lock mechanism would not need to be changed
but would always be effective, even in single-threaded programs.

This way old code that calls Fl::lock() works as before and code that
does not call Fl::lock() would still use unlock/lock internally and
could use threads with FLTK locking etc.

That said, I think it's possible to do, but I'd still vote for leaving
everything as-is, i.e. do not force single-threaded apps to do internal
locking.