Temporarily yielding SolarMutex to another thread

classic Classic list List threaded Threaded
5 messages Options
Luboš Luňák Luboš Luňák
Reply | Threaded
Open this post in threaded view
|

Temporarily yielding SolarMutex to another thread


 Hello,

 does somebody know of a (solution) of a case when it would be useful to make
another thread temporarily act if it held the SolarMutex? I've recently ran
into 2 such cases:

- Calc threading - the Interpret() call may spawn several threads to compute
in parallel a whole formula group. SolarMutex is still held by the main
thread and the child threads generally try to avoid locking (it's ensured
beforehand they will operate on distinct data). However, some functionality
requires SolarMutex and those operations are currently blacklisted for
threading, since this would deadlock. The main thread cannot release
SolarMutex just like that in the middle of a computation, another independent
thread could possibly access a state that's inconsistent at the moment.
However, the main thread is blocked waiting for its child threads and so
semantically all those threads "own" the SolarMutex, the child threads are
there simply to use all CPU cores. I even remember seeing a piece of code
which simply "silently" (with just a log warning) bails out if it can't
acquire the SolarMutex.

- Clipboard handling on Windows - I'm a bit hazy on the technical details, but
if I'm getting it right, OLE requires all clipboard handling to be done by
one thread, which our clipboard code does by creating a dedicated clipboard
thread. Most clipboard operations ask the clipboard thread to do the job and
block waiting for it, but setting clipboard content is currently
asynchronous, because decoding the set content requires SolarMutex, and again
trying to acquire it from the clipboard thread would deadlock if the main
thread holding it would block waiting for the clipboard thread. But the
current situation with delayed handling can lead to previous clipboard
contents freed too late, causing a crash. And again, this is really just one
thread asking another one to do an operation, so again the clipboard thread
could be semantically considered an owner of SolarMutex during such an
operation.

 So, has somebody already handled a case like this? And is there a better way
of handling this than adding some explicit ownership-yield operation to
SolarMutex? It sounds a bit dirty to do something like that, but I can't
think of anything better.

--
 Luboš Luňák
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Noel Grandin-2 Noel Grandin-2
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily yielding SolarMutex to another thread



On Fri, 1 Feb 2019 at 17:11, Luboš Luňák <[hidden email]> wrote:
- Calc threading - the Interpret() call may spawn several threads to compute

This case smells like this functionality should be using a different mutex instead of the SolarMutex, then the main thread could hold some kind of InterpretMutex, and the child threads could acquire and release the SolarMutex as necessary.

Not ideal in terms of overhead, but less likelyhood of something going wrong in the future.



- Clipboard handling on Windows - I'm a bit hazy on the technical details, but
if I'm getting it right, OLE requires all clipboard handling to be done by
one thread, which our clipboard code does by creating a dedicated clipboard
thread. Most clipboard operations ask the clipboard thread to do the job and
block waiting for it,

Perhaps the calling thread should pass both an operation and a std::function to the clipboard thread, which the clipboard thread can post back to the event thread once it's done - kind of a poor mans continuation.
but setting clipboard content is currently
asynchronous, because decoding the set content requires SolarMutex, and again
trying to acquire it from the clipboard thread would deadlock if the main
thread holding it would block waiting for the clipboard thread. But the
current situation with delayed handling can lead to previous clipboard
contents freed too late, causing a crash.

"freed too late" ? sounds like an ownership issue, not a threading issue?

I'm sure it is possible to get clever with mutexes, but I suspect you'd just be setting yourself up for bugs that are even harder to reason about.


_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Luboš Luňák Luboš Luňák
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily yielding SolarMutex to another thread

On Friday 01 of February 2019, Noel Grandin wrote:
> On Fri, 1 Feb 2019 at 17:11, Luboš Luňák <[hidden email]> wrote:
> > - Calc threading - the Interpret() call may spawn several threads to
> > compute
>
> This case smells like this functionality should be using a different mutex
> instead of the SolarMutex, then the main thread could hold some kind of
> InterpretMutex, and the child threads could acquire and release the
> SolarMutex as necessary.

 That wouldn't work. As I said, the main thread cannot release SolarMutex at
that point, since any other thread could acquire it there. And as long as the
main thread cannot release SolarMutex, no other thread can acquire it, not
even the child ones. And as long as the child threads cannot own SolarMutex,
they cannot use all kinds of LO functionality that require it, not just Calc
code, but anywhere.

> Perhaps the calling thread should pass both an operation and a
> std::function to the clipboard thread, which the clipboard thread can post
> back to the event thread once it's done - kind of a poor mans continuation.

 We have places locking SolarMutex all over the codebase. That'd be such a
mess to do it from places that the child thread could possibly reach. Playing
tricks with mutexes would be a nice clean approach compared to that.

> "freed too late" ? sounds like an ownership issue, not a threading issue?

 It's roughly like this: There's SdrModel and some SdrObject instances, which
refer to the SdrModel. Some SdrObject may get added to clipboard contents.
Later, when it all is about to get deleted, first all SdrObject instances get
deleted from whatever may refer to it, which includes resetting clipboard
contents to something that no longer refers to it. And when all that is done,
SdrModel is destroyed too.

 Except, Windows clipboard handling decided it would be fun to delay resetting
clipboard contents, and so one SdrObject actually gets destroyed only later,
when the clipboard code finally gets to it. SdrObject dtor refers to the no
longer existing SdrModel, *boom*.

 The way I see it, this is clearly a fault of the Windows clipboard code.

> I'm sure it is possible to get clever with mutexes, but I suspect you'd
> just be setting yourself up for bugs that are even harder to reason about.

 I disagree. Getting clever with mutexes indeed looks very wrong
theoretically, but in practice it'd be a hack that'd be limited to a small
scope. Assuming it doesn't deadlock (which should be easy to spot) and
assuming it doesn't have a performance impact, what else can go wrong in
practice?

--
 Luboš Luňák
 [hidden email]
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Noel Grandin-2 Noel Grandin-2
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily yielding SolarMutex to another thread


Assuming that there is no other way to accomplish this, how could it work to allow multiple child widgets to "own" the SolarMutex simultaneously? We just need one of those threads to trigger an update to something like a cache, and one of the other threads is likely to crash because of a stale pointer.

Unless you are talking about passing some kind of permission down to the child threads, which says "this thread is allowed to also take the SolarMutex, even though it is already locked", which means we'd need another counter, and then how would that interact with SolarMutexReleaser.

Possibly another way here is to make the parts of the child threads that currently need the SolarMutex, to get their own mutex, preferably a reader/writer mutex, so that multiple of them can run in parallel.


_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice
Jan-Marek Glogowski Jan-Marek Glogowski
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily yielding SolarMutex to another thread

Am 2. Februar 2019 14:49:29 MEZ schrieb Noel Grandin <[hidden email]>:

>Assuming that there is no other way to accomplish this, how could it
>work
>to allow multiple child widgets to "own" the SolarMutex simultaneously?
>We
>just need one of those threads to trigger an update to something like a
>cache, and one of the other threads is likely to crash because of a
>stale
>pointer.
>
>Unless you are talking about passing some kind of permission down to
>the
>child threads, which says "this thread is allowed to also take the
>SolarMutex, even though it is already locked", which means we'd need
>another counter, and then how would that interact with
>SolarMutexReleaser.
>
>Possibly another way here is to make the parts of the child threads
>that
>currently need the SolarMutex, to get their own mutex, preferably a
>reader/writer mutex, so that multiple of them can run in parallel.

The only real fix is to get rid of SolarMutex (SM) ;-)
While that is a long term goal, the way to do this is IMHO to keep the SM in the parent thread and use some messaging to transfer SM work from child to parent.

Your idea is basically what the OSX VCL backend is doing: transfer work via code block passing and a flag to ignore SM in the main thread while working on the code block in main. Have a look at https://cgit.freedesktop.org/libreoffice/core/tree/vcl/inc/osx/runinmain.hxx

In VCL we kind of do this on all platforms using different mechanisms, as all of them require to do GUI stuff in the main thread (unless you use threaded OpenGL), except GTK+, which also has a kind of SM (GDK lock), as it has to honour the platforms constraints anyway.

All the non-GUI work should use its own synchronisation primitive.
_______________________________________________
LibreOffice mailing list
[hidden email]
https://lists.freedesktop.org/mailman/listinfo/libreoffice