Discussion:
[LyX/master] Make theWordList() thread safe.
Georg Baum
2014-07-05 08:40:20 UTC
Permalink
commit cf89851374163be2e8a390dd917d7ef435978979
Date: Fri Jul 4 22:19:43 2014 +0200
Make theWordList() thread safe.
Without this, you get crashes in a few second when you set the
autosave interval to one second and edit quickly (typing new words
etc). The reason is that the cloned buffer wants to insert words into
the word list and remove them again, but it lives in a different
thread.
This should go to the 2.1 branch after 2.1.1 is released IMHO. This fixes a
crash that can easily be triggered by autosave (although not the one we are
hunting).


Georg
Richard Heck
2014-07-06 14:30:49 UTC
Permalink
Post by Georg Baum
commit cf89851374163be2e8a390dd917d7ef435978979
Date: Fri Jul 4 22:19:43 2014 +0200
Make theWordList() thread safe.
Without this, you get crashes in a few second when you set the
autosave interval to one second and edit quickly (typing new words
etc). The reason is that the cloned buffer wants to insert words into
the word list and remove them again, but it lives in a different
thread.
This should go to the 2.1 branch after 2.1.1 is released IMHO. This fixes a
crash that can easily be triggered by autosave (although not the one we are
hunting).
Go ahead. Branch is open again.

rh
Richard Heck
2014-07-06 14:37:18 UTC
Permalink
commit cf89851374163be2e8a390dd917d7ef435978979
Date: Fri Jul 4 22:19:43 2014 +0200
Make theWordList() thread safe.
Without this, you get crashes in a few second when you set the autosave
interval to one second and edit quickly (typing new words etc). The reason
is that the cloned buffer wants to insert words into the word list and
remove them again, but it lives in a different thread.
Good catch. That said, clones are wasting their (and our!) time
inserting things into the WordList. Would it be hard to stop them from
bothering?

Richard
Richard Heck
2014-07-06 14:41:42 UTC
Permalink
Post by Richard Heck
commit cf89851374163be2e8a390dd917d7ef435978979
Date: Fri Jul 4 22:19:43 2014 +0200
Make theWordList() thread safe.
Without this, you get crashes in a few second when you set the autosave
interval to one second and edit quickly (typing new words etc). The reason
is that the cloned buffer wants to insert words into the word list and
remove them again, but it lives in a different thread.
Good catch. That said, clones are wasting their (and our!) time
inserting things into the WordList. Would it be hard to stop them
from bothering?
How many of the other thread safety commits do you think are needed in
branch? I'm kind of guessing they'd all be nice...

Richard
Georg Baum
2014-07-06 16:23:53 UTC
Permalink
Post by Richard Heck
How many of the other thread safety commits do you think are needed in
branch? I'm kind of guessing they'd all be nice...
This one was the only one where I realy saw a crash. From the remaining ones
2a677592 and 5093893b5 also fixed other bugs, and 5a8b8ba8 is important, so
I propose these as well. For the remaining ones I don't know: I am pretty
sure that these fixes are correct, but they are morre exotic. What do others
think?


Georg
Kornel Benko
2014-07-06 19:03:14 UTC
Permalink
Post by Georg Baum
Post by Richard Heck
How many of the other thread safety commits do you think are needed in
branch? I'm kind of guessing they'd all be nice...
This one was the only one where I realy saw a crash. From the remaining ones
2a677592 and 5093893b5 also fixed other bugs, and 5a8b8ba8 is important, so
I propose these as well. For the remaining ones I don't know: I am pretty
sure that these fixes are correct, but they are morre exotic. What do others
think?
I think all of them should go to branch as well.
Post by Georg Baum
Georg
Kornel
Jürgen Spitzmüller
2014-07-07 05:46:25 UTC
Permalink
Post by Kornel Benko
Post by Georg Baum
This one was the only one where I realy saw a crash. From the remaining
ones 2a677592 and 5093893b5 also fixed other bugs, and 5a8b8ba8 is
important, so I propose these as well. For the remaining ones I don't
know: I am pretty sure that these fixes are correct, but they are morre
exotic. What do others think?
I think all of them should go to branch as well.
Me, too. Thanks for this work, BTW.

Jürgen
Richard Heck
2014-07-07 08:58:24 UTC
Permalink
Post by Jürgen Spitzmüller
Post by Kornel Benko
Post by Georg Baum
This one was the only one where I realy saw a crash. From the remaining
ones 2a677592 and 5093893b5 also fixed other bugs, and 5a8b8ba8 is
important, so I propose these as well. For the remaining ones I don't
know: I am pretty sure that these fixes are correct, but they are morre
exotic. What do others think?
I think all of them should go to branch as well.
Me, too. Thanks for this work, BTW.
Go ahead, then, Georg. (And thanks also from me.)

Richard
Georg Baum
2014-07-07 19:26:47 UTC
Permalink
Post by Richard Heck
Post by Jürgen Spitzmüller
Post by Kornel Benko
Post by Georg Baum
This one was the only one where I realy saw a crash. From the remaining
ones 2a677592 and 5093893b5 also fixed other bugs, and 5a8b8ba8 is
important, so I propose these as well. For the remaining ones I don't
know: I am pretty sure that these fixes are correct, but they are morre
exotic. What do others think?
I think all of them should go to branch as well.
Me, too. Thanks for this work, BTW.
Go ahead, then, Georg. (And thanks also from me.)
OK, I'll merge all fixes then. You are all welcome. It was quite easy btw:
We have two needed tools (a mutex and local thread storage), so my algorithm
was:

1) search for FIXME THREAD, pick one that looks interesting
2) decide what to do: Is it possible to get rid of the static data
completely? If not, is it better to use a mutex or local thread storage?
3) implement and test the solution
4) goto 1) until I could not be bothered anymore

Unfortunately some of the remaining FIXME THREAD occurances are more
difficult. I might have a look at them as well, but I don't know yet.


Georg
Scott Kostyshak
2014-07-07 20:01:32 UTC
Permalink
On Mon, Jul 7, 2014 at 3:26 PM, Georg Baum
Post by Georg Baum
2) decide what to do: Is it possible to get rid of the static data
completely? If not, is it better to use a mutex or local thread storage?
How do you decide between a mutex or local thread storage? My wild
guess is (I have no experience with concurrency issues but have wanted
to study them): local thread storage is better if threads do not need
to share memory between them.

Scott
Georg Baum
2014-07-07 20:33:07 UTC
Permalink
Post by Scott Kostyshak
How do you decide between a mutex or local thread storage? My wild
guess is (I have no experience with concurrency issues but have wanted
to study them): local thread storage is better if threads do not need
to share memory between them.
Basically yes. In some cases there is no choice, e.g. in 0de4bc224af3: The
mangled name must be the same for any given input name, regardless of the
thread => use mutex. Another case is the wordlist: Cloned buffers must not
disturb the wordlist of any other buffer => use thread local storage. In
other cases it does not really matter, e.g. 4a2250a.


Georg
Scott Kostyshak
2014-07-09 06:35:16 UTC
Permalink
On Mon, Jul 7, 2014 at 4:33 PM, Georg Baum
In other cases it does not really matter, e.g. 4a2250a.
I think I understand more now. But why in this commit do you choose a
mutex? Why is it obvious that "a deadlock can't happen" ? And why "it
should give better performance" ?

In terms of efficiency, in general if you can use either a mutex or
local thread storage, if the object is light it seems like local
thread storage would be more efficient because no thread would ever
have to wait to get access to the shared object. But if the object is
heavy, then maybe the overhead of creating it in multiple threads is
more than the time saved from threads waiting their turn. Is that at
all correct?

Scott
Georg Baum
2014-07-09 20:01:01 UTC
Permalink
Post by Scott Kostyshak
I think I understand more now. But why in this commit do you choose a
mutex? Why is it obvious that "a deadlock can't happen" ? And why "it
should give better performance" ?
A deadlock would mean that thread x has the lock and waits for thread y,
while thread y waits for thread x. This can't happen, because

a) Formats::isZippedFile() does not call any method or function that
communicates in any way with other threads, and

b) it does not use any shared resource. Even if one tries to construct a
situation where the file being queried in thread x is currently opened
exclusively in another thread or process, and thread y enters
isZippedFile(), you may get wrong information about the zipped status in
thread x, but no deadlock. The wrong information is caused by the missing
error handling of guessFormatFromContents(), and this problem would even
exist in a single threaded LyX.

Therefore, if one thread enters isZippedFile(), it will eventually finish
without waiting for any other thread, so the worst thing that happens if two
threads enter isZippedFile() is that one has to wait.

The better performance is only my guess. My line of thought was this:
getFormatFromFile() is expensive (otherwise the whole cache would not be
needed at all), and the probability that two threads arrive at the same time
in isZippedFile() is low, and the cost of obtaining a lock without conflict
is low as well => share the result of getFormatFromFile() between threads.
As always when you want to know something about performance, you should
measure, but this can be quite difficult, and we are not talking about an
inner loop of a complicated numerical procedure, so I was lazy and did not
measure.
Post by Scott Kostyshak
In terms of efficiency, in general if you can use either a mutex or
local thread storage, if the object is light it seems like local
thread storage would be more efficient because no thread would ever
have to wait to get access to the shared object. But if the object is
heavy, then maybe the overhead of creating it in multiple threads is
more than the time saved from threads waiting their turn. Is that at
all correct?
Basically yes, but you need to take into account the computation of the
object as well: The object might be very light (in this case it is a bool),
but the computation might be quite expensive. memory-wise it would not be
any problem to use thread-local data, but computation time wise you would
see a difference.

In LyX we have a quite comfortable situation: It is a small project (so you
can check for deadlocks easily in many cases), and it has only a few
performance-critical areas, so in many cases it does not matter if we take
the wrong decision performance wise;-)



Georg
Scott Kostyshak
2014-07-10 04:56:59 UTC
Permalink
On Wed, Jul 9, 2014 at 4:01 PM, Georg Baum
Post by Georg Baum
Post by Scott Kostyshak
I think I understand more now. But why in this commit do you choose a
mutex? Why is it obvious that "a deadlock can't happen" ? And why "it
should give better performance" ?
A deadlock would mean that thread x has the lock and waits for thread y,
while thread y waits for thread x. This can't happen, because
a) Formats::isZippedFile() does not call any method or function that
communicates in any way with other threads, and
b) it does not use any shared resource. Even if one tries to construct a
situation where the file being queried in thread x is currently opened
exclusively in another thread or process, and thread y enters
isZippedFile(), you may get wrong information about the zipped status in
thread x, but no deadlock. The wrong information is caused by the missing
error handling of guessFormatFromContents(), and this problem would even
exist in a single threaded LyX.
Therefore, if one thread enters isZippedFile(), it will eventually finish
without waiting for any other thread, so the worst thing that happens if two
threads enter isZippedFile() is that one has to wait.
getFormatFromFile() is expensive (otherwise the whole cache would not be
needed at all), and the probability that two threads arrive at the same time
in isZippedFile() is low, and the cost of obtaining a lock without conflict
is low as well => share the result of getFormatFromFile() between threads.
As always when you want to know something about performance, you should
measure, but this can be quite difficult, and we are not talking about an
inner loop of a complicated numerical procedure, so I was lazy and did not
measure.
Post by Scott Kostyshak
In terms of efficiency, in general if you can use either a mutex or
local thread storage, if the object is light it seems like local
thread storage would be more efficient because no thread would ever
have to wait to get access to the shared object. But if the object is
heavy, then maybe the overhead of creating it in multiple threads is
more than the time saved from threads waiting their turn. Is that at
all correct?
Basically yes, but you need to take into account the computation of the
object as well: The object might be very light (in this case it is a bool),
but the computation might be quite expensive. memory-wise it would not be
any problem to use thread-local data, but computation time wise you would
see a difference.
In LyX we have a quite comfortable situation: It is a small project (so you
can check for deadlocks easily in many cases), and it has only a few
performance-critical areas, so in many cases it does not matter if we take
the wrong decision performance wise;-)
Thanks, Georg. I really appreciate your explanation!

Scott
Richard Heck
2014-07-10 17:49:03 UTC
Permalink
Post by Scott Kostyshak
On Wed, Jul 9, 2014 at 4:01 PM, Georg Baum
Post by Georg Baum
Post by Scott Kostyshak
I think I understand more now. But why in this commit do you choose a
mutex? Why is it obvious that "a deadlock can't happen" ? And why "it
should give better performance" ?
A deadlock would mean that thread x has the lock and waits for thread y,
while thread y waits for thread x. This can't happen, because
a) Formats::isZippedFile() does not call any method or function that
communicates in any way with other threads, and
b) it does not use any shared resource. Even if one tries to construct a
situation where the file being queried in thread x is currently opened
exclusively in another thread or process, and thread y enters
isZippedFile(), you may get wrong information about the zipped status in
thread x, but no deadlock. The wrong information is caused by the missing
error handling of guessFormatFromContents(), and this problem would even
exist in a single threaded LyX.
Therefore, if one thread enters isZippedFile(), it will eventually finish
without waiting for any other thread, so the worst thing that happens if two
threads enter isZippedFile() is that one has to wait.
getFormatFromFile() is expensive (otherwise the whole cache would not be
needed at all), and the probability that two threads arrive at the same time
in isZippedFile() is low, and the cost of obtaining a lock without conflict
is low as well => share the result of getFormatFromFile() between threads.
As always when you want to know something about performance, you should
measure, but this can be quite difficult, and we are not talking about an
inner loop of a complicated numerical procedure, so I was lazy and did not
measure.
Post by Scott Kostyshak
In terms of efficiency, in general if you can use either a mutex or
local thread storage, if the object is light it seems like local
thread storage would be more efficient because no thread would ever
have to wait to get access to the shared object. But if the object is
heavy, then maybe the overhead of creating it in multiple threads is
more than the time saved from threads waiting their turn. Is that at
all correct?
Basically yes, but you need to take into account the computation of the
object as well: The object might be very light (in this case it is a bool),
but the computation might be quite expensive. memory-wise it would not be
any problem to use thread-local data, but computation time wise you would
see a difference.
In LyX we have a quite comfortable situation: It is a small project (so you
can check for deadlocks easily in many cases), and it has only a few
performance-critical areas, so in many cases it does not matter if we take
the wrong decision performance wise;-)
Thanks, Georg. I really appreciate your explanation!
Me too!

Richard

Georg Baum
2014-07-06 16:19:06 UTC
Permalink
Post by Richard Heck
Good catch. That said, clones are wasting their (and our!) time
inserting things into the WordList. Would it be hard to stop them from
bothering?
Yes, that would be better. Unfortunately I am not familiar with the clone
machinery nor the wordlist, so I did nothing more than making the existing
code threadsafe.


Georg
Loading...