Re: Pandora's box

[Guillaume Munch]

Le 06/08/2016 à 10:31, Abdelrazak Younes a écrit :

Hi Guillaume,

Just my 2 cents about those recent commits:

1) Switching to C++11 thread is a good thing
2) All these #if (gcc 4.6) are uglyfying the code considerably, you
should enter C++11 with 2 steps or just leave it.

Thanks,
Abdel




Hi Abdel,

I do not really like having to use this, but this was the only way to
fix this now, and even then I allowed it only because I hope to get rid
of all these branches as soon as nobody wants gcc 4.6 anymore. Anyway,
the ones I just added were reverted for a different reason now.

Thanks
Guillaume

Re: Pandora's box

[Abdelrazak Younes]

Hi Guillaume,

Just my 2 cents about those recent commits:

1) Switching to C++11 thread is a good thing
2) All these #if (gcc 4.6) are uglyfying the code considerably, you 
should enter C++11 with 2 steps or just leave it.


Thanks,
Abdel

On 31/07/2016 19:38, Guillaume Munch wrote:

Hi list,


By touching upon the problem of static variables and threading in LyX
(see my patch at <http://mid.gmane.org/nltsru$pag$1...@ger.gmane.org>), I
have the impression that I opened Pandora's box.

I just committed a series of patches that fixes "FIXME thread" in cases
where c++11 gives an easy solution for common problems, with some
explanations which you may enjoy.

Here's an additional patch that uses thread_local instead of static to
fix, I think, threading issues for some of the cases (e.g. when caching
some value for optimisation purpose). But I'd like to hear some comments
before committing.

For other "FIXME thread" issues, either the solution is more complex, or
using statics is inappropriate altogether (I think). Is there a plan to
get rid of these?

Also, when reviewing the use of non-const static variables, I found many
cases where thread-safety was not obvious to me but no "FIXME thread"
comment was present. Is it known whether the remaining static variables
without "FIXME thread" are thread-safe?

Ideally, I think it would be nice to only use statics when necessary,
and use the new tools provided by C++11 (see my recent commits and the
attached patch).


Guillaume

Re: Pandora's box

[Richard Heck]

On 08/01/2016 06:50 PM, Guillaume Munch wrote:
> Le 31/07/2016 à 19:52, Richard Heck a écrit :
>>> +++ b/src/frontends/qt4/GuiCitation.cpp
>>
>> Is there some way we could store these per BufferView or something? I'd
>> kind of like to save the last searched string for each document,
>> rather than
>> globally.
>
> Yes, I think there must be a better solution.

I suppose for now it would be fine to go with the easy one, and leave a
FIXME.

>>
>> PS We have:
>> static vector cache_set(NUM_FAMILIES, false);
>> cache_set[family] = true;
>> ??
>
> vector has a specific implementation, so maybe it used to cause
> issues on some compliers for some reason. It works fine here.

Yes, I thought of that later. I seem to remember once reading somewhere
that one should avoid vector

rh

Re: Pandora's box

[Richard Heck]

On 08/01/2016 04:01 PM, Guillaume Munch wrote:
> Le 31/07/2016 à 19:52, Richard Heck a écrit :
>> On 07/31/2016 01:38 PM, Guillaume Munch wrote:
>>
>>> For other "FIXME thread" issues, either the solution is more
>>> complex, or
>>> using statics is inappropriate altogether (I think). Is there a plan to
>>> get rid of these?
>>
>> It looks to me as if most of the ones that are left are in
>> src/frontends/. It would
>> surely be worth fixing these, but I doubt they actually cause problems,
>> at least as
>> the code is now. We only ever have one GUI thread. For thar reason, the
>> switch
>> from static to thread_local in GuiDocument, say, will have no practical
>> effect at
>> the moment.
>>
>
> To me this sounds like an argument for such changes: it preserves the
> current behaviour, and will ensure that it remains thread-safe in the
> future.

Didn't mean to suggest otherwise.

rh

Re: Pandora's box

[Guillaume Munch]

Le 31/07/2016 à 19:52, Richard Heck a écrit :

+++ b/src/frontends/qt4/GuiCitation.cpp


Is there some way we could store these per BufferView or something? I'd
kind of
like to save the last searched string for each document, rather than
globally.


Yes, I think there must be a better solution.


+++ b/src/frontends/qt4/GuiDocument.cpp


I think this cache could just be removed. It's used when we validate
listings parameters,
which is (a) very rarely and (b) a GUI-related operation, which means
that the very minor
speed optimization involved will not be noticed.


Agreed.


+++ b/src/frontends/qt4/GuiFontLoader.cpp
+   thread_local vector cache_set(NUM_FAMILIES, false);


Here, I think we'd prefer to have something actually global (although,
as I said, this
change will not now have any practical effect). Can the "atomic" stuff
be used to allow
us to do that?


It is not possible to have an atomic vector/array because atomics only
work for trivially copyable types. Then, I started reading about
vector/arrays of atomics in c++ and this started giving me a headache.
So I am going to reply that having a thread_local cache is the simplest
solution.



PS We have:
static vector cache_set(NUM_FAMILIES, false);
cache_set[family] = true;
??


vector has a specific implementation, so maybe it used to cause 
issues on some compliers for some reason. It works fine here.

Re: Pandora's box

[Guillaume Munch]

Le 31/07/2016 à 19:52, Richard Heck a écrit :

On 07/31/2016 01:38 PM, Guillaume Munch wrote:


For other "FIXME thread" issues, either the solution is more complex, or
using statics is inappropriate altogether (I think). Is there a plan to
get rid of these?


It looks to me as if most of the ones that are left are in
src/frontends/. It would
surely be worth fixing these, but I doubt they actually cause problems,
at least as
the code is now. We only ever have one GUI thread. For thar reason, the
switch
from static to thread_local in GuiDocument, say, will have no practical
effect at
the moment.



To me this sounds like an argument for such changes: it preserves the
current behaviour, and will ensure that it remains thread-safe in the
future.

Guillaume

Re: Pandora's box

[Richard Heck]

Patch comments.

> From 7fd53bc44bd278fd1f30400c030e350f2c0a6aa3 Mon Sep 17 00:00:00 2001
> From: Guillaume Munch 
> Date: Sun, 31 Jul 2016 00:42:51 +0100
> Subject: [PATCH] Replace static with thread_local when used for caching
>
> thread_local is a per-thread static variable, so it is thread-safe and can be
> used for caching purpose.
>
> (requires gcc >= 4.8)
> ---
>  src/frontends/qt4/GuiCitation.cpp   | 11 ---
>  src/frontends/qt4/GuiDocument.cpp   |  7 ++-
>  src/frontends/qt4/GuiFontLoader.cpp |  6 +-
>  src/frontends/qt4/GuiInclude.cpp| 12 
>  src/frontends/qt4/GuiListings.cpp   |  9 +++--
>  src/frontends/qt4/GuiPainter.cpp|  7 ++-
>  src/frontends/qt4/GuiSymbols.cpp|  5 -
>  7 files changed, 44 insertions(+), 13 deletions(-)
>
> diff --git a/src/frontends/qt4/GuiCitation.cpp 
> b/src/frontends/qt4/GuiCitation.cpp
> index 028e874..f209a24 100644
> --- a/src/frontends/qt4/GuiCitation.cpp
> +++ b/src/frontends/qt4/GuiCitation.cpp
> @@ -570,12 +570,17 @@ void GuiCitation::findKey(BiblioInfo const & bi,
>   docstring field, docstring entry_type,
>   bool case_sensitive, bool reg_exp, bool reset)
>  {
> - // FIXME THREAD
> - // Used for optimisation: store last searched string.
> +#if defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6)
>   static QString last_searched_string;
> - // Used to disable the above optimisation.
>   static bool last_case_sensitive;
>   static bool last_reg_exp;
> +#else
> + // Used for optimisation: store last searched string.
> + thread_local QString last_searched_string;
> + // Used to disable the above optimisation.
> + thread_local bool last_case_sensitive;
> + thread_local bool last_reg_exp;
> +#endif

Is there some way we could store these per BufferView or something? I'd
kind of
like to save the last searched string for each document, rather than
globally.

> diff --git a/src/frontends/qt4/GuiDocument.cpp 
> b/src/frontends/qt4/GuiDocument.cpp
> index e116001..c98332e 100644
> --- a/src/frontends/qt4/GuiDocument.cpp
> +++ b/src/frontends/qt4/GuiDocument.cpp
> @@ -1498,9 +1498,14 @@ QString GuiDocument::validateListingsParameters()
>  {
>   // use a cache here to avoid repeated validation
>   // of the same parameters
> - // FIXME THREAD
> + // FIXME code duplication with GuiInclude and GuiListings
> +#if defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6)
>   static string param_cache;
>   static QString msg_cache;
> +#else
> + thread_local string param_cache;
> + thread_local QString msg_cache;
> +#endif

I think this cache could just be removed. It's used when we validate
listings parameters,
which is (a) very rarely and (b) a GUI-related operation, which means
that the very minor
speed optimization involved will not be noticed.

>   return QString();
> diff --git a/src/frontends/qt4/GuiFontLoader.cpp 
> b/src/frontends/qt4/GuiFontLoader.cpp
> index cc092f5..45392d8 100644
> --- a/src/frontends/qt4/GuiFontLoader.cpp
> +++ b/src/frontends/qt4/GuiFontLoader.cpp
> @@ -373,9 +373,13 @@ GuiFontInfo::GuiFontInfo(FontInfo const & f)
>  
>  bool FontLoader::available(FontInfo const & f)
>  {
> - // FIXME THREAD
> +#if defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6)
>   static vector cache_set(NUM_FAMILIES, false);
>   static vector cache(NUM_FAMILIES, false);
> +#else
> + thread_local vector cache_set(NUM_FAMILIES, false);
> + thread_local vector cache(NUM_FAMILIES, false);
> +#endif

Here, I think we'd prefer to have something actually global (although,
as I said, this
change will not now have any practical effect). Can the "atomic" stuff
be used to allow
us to do that?

PS We have:
static vector cache_set(NUM_FAMILIES, false);
cache_set[family] = true;
??

> diff --git a/src/frontends/qt4/GuiInclude.cpp 
> b/src/frontends/qt4/GuiInclude.cpp
> index e1fc275..2fe7f8f 100644
> --- a/src/frontends/qt4/GuiInclude.cpp
> +++ b/src/frontends/qt4/GuiInclude.cpp
> @@ -93,10 +93,14 @@ docstring GuiInclude::validate_listings_params()
>  {
>   // use a cache here to avoid repeated validation
>   // of the same parameters
> - // FIXME THREAD
> - static string param_cache = string();
> - static docstring msg_cache = docstring();
> - 
> +#if defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6)
> + static string param_cache;
> + static docstring msg_cache;
> +#else
> + thread_local string param_cache;
> + thread_local docstring msg_cache;
> +#endif
> +

As above, I think this cachee could just be removed.

> diff --git a/src/frontends/qt4/GuiListings.cpp 
> b/src/frontends/qt4/GuiListings.cpp
> index da12882..b9edeb3 100644
> --- a/src/frontends/qt4/GuiListings.cpp
> +++ b/src/frontends/qt4/GuiListings.cpp
> @@ -348,10 +348,15 @@ docstring GuiListings::validate_listings_params()
>  {
>   // use a cache here to avoid 

Re: Pandora's box

[Richard Heck]

On 07/31/2016 01:38 PM, Guillaume Munch wrote:
> Hi list,
>
> By touching upon the problem of static variables and threading in LyX
> (see my patch at <http://mid.gmane.org/nltsru$pag$1...@ger.gmane.org>), I
> have the impression that I opened Pandora's box.

I think the URL there is broken.

> I just committed a series of patches that fixes "FIXME thread" in cases
> where c++11 gives an easy solution for common problems, with some
> explanations which you may enjoy.

Thanks. There are surely a lot of hidden problems here. The LyX code was
not originally written with thread-safety in mind. It was only when
background
export was added that these issues became relevant.

> For other "FIXME thread" issues, either the solution is more complex, or
> using statics is inappropriate altogether (I think). Is there a plan to
> get rid of these?

It looks to me as if most of the ones that are left are in
src/frontends/. It would
surely be worth fixing these, but I doubt they actually cause problems,
at least as
the code is now. We only ever have one GUI thread. For thar reason, the
switch
from static to thread_local in GuiDocument, say, will have no practical
effect at
the moment.

> Also, when reviewing the use of non-const static variables, I found many
> cases where thread-safety was not obvious to me but no "FIXME thread"
> comment was present. Is it known whether the remaining static variables
> without "FIXME thread" are thread-safe?

I went through and marked these some time ago. I believe I checked all
static
variables, and the ones I did not mark I guess I thought were safe. I
ought to
have added a comment in such cases.

> Ideally, I think it would be nice to only use statics when necessary,
> and use the new tools provided by C++11 (see my recent commits and the
> attached patch).

Also probably worth doing.

Richard

Pandora's box

[Guillaume Munch]

Hi list,


By touching upon the problem of static variables and threading in LyX
(see my patch at <http://mid.gmane.org/nltsru$pag$1...@ger.gmane.org>), I
have the impression that I opened Pandora's box.

I just committed a series of patches that fixes "FIXME thread" in cases
where c++11 gives an easy solution for common problems, with some
explanations which you may enjoy.

Here's an additional patch that uses thread_local instead of static to
fix, I think, threading issues for some of the cases (e.g. when caching
some value for optimisation purpose). But I'd like to hear some comments
before committing.

For other "FIXME thread" issues, either the solution is more complex, or
using statics is inappropriate altogether (I think). Is there a plan to
get rid of these?

Also, when reviewing the use of non-const static variables, I found many
cases where thread-safety was not obvious to me but no "FIXME thread"
comment was present. Is it known whether the remaining static variables
without "FIXME thread" are thread-safe?

Ideally, I think it would be nice to only use statics when necessary,
and use the new tools provided by C++11 (see my recent commits and the
attached patch).


Guillaume
>From 7fd53bc44bd278fd1f30400c030e350f2c0a6aa3 Mon Sep 17 00:00:00 2001
From: Guillaume Munch <g...@lyx.org>
Date: Sun, 31 Jul 2016 00:42:51 +0100
Subject: [PATCH] Replace static with thread_local when used for caching

thread_local is a per-thread static variable, so it is thread-safe and can be
used for caching purpose.

(requires gcc >= 4.8)
---
 src/frontends/qt4/GuiCitation.cpp   | 11 ---
 src/frontends/qt4/GuiDocument.cpp   |  7 ++-
 src/frontends/qt4/GuiFontLoader.cpp |  6 +-
 src/frontends/qt4/GuiInclude.cpp| 12 
 src/frontends/qt4/GuiListings.cpp   |  9 +++--
 src/frontends/qt4/GuiPainter.cpp|  7 ++-
 src/frontends/qt4/GuiSymbols.cpp|  5 -
 7 files changed, 44 insertions(+), 13 deletions(-)

diff --git a/src/frontends/qt4/GuiCitation.cpp b/src/frontends/qt4/GuiCitation.cpp
index 028e874..f209a24 100644
--- a/src/frontends/qt4/GuiCitation.cpp
+++ b/src/frontends/qt4/GuiCitation.cpp
@@ -570,12 +570,17 @@ void GuiCitation::findKey(BiblioInfo const & bi,
 	docstring field, docstring entry_type,
 	bool case_sensitive, bool reg_exp, bool reset)
 {
-	// FIXME THREAD
-	// Used for optimisation: store last searched string.
+#if defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6)
 	static QString last_searched_string;
-	// Used to disable the above optimisation.
 	static bool last_case_sensitive;
 	static bool last_reg_exp;
+#else
+	// Used for optimisation: store last searched string.
+	thread_local QString last_searched_string;
+	// Used to disable the above optimisation.
+	thread_local bool last_case_sensitive;
+	thread_local bool last_reg_exp;
+#endif
 	// Reset last_searched_string in case of changed option.
 	if (last_case_sensitive != case_sensitive
 		|| last_reg_exp != reg_exp) {
diff --git a/src/frontends/qt4/GuiDocument.cpp b/src/frontends/qt4/GuiDocument.cpp
index e116001..c98332e 100644
--- a/src/frontends/qt4/GuiDocument.cpp
+++ b/src/frontends/qt4/GuiDocument.cpp
@@ -1498,9 +1498,14 @@ QString GuiDocument::validateListingsParameters()
 {
 	// use a cache here to avoid repeated validation
 	// of the same parameters
-	// FIXME THREAD
+	// FIXME code duplication with GuiInclude and GuiListings
+#if defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6)
 	static string param_cache;
 	static QString msg_cache;
+#else
+	thread_local string param_cache;
+	thread_local QString msg_cache;
+#endif
 
 	if (listingsModule->bypassCB->isChecked())
 		return QString();
diff --git a/src/frontends/qt4/GuiFontLoader.cpp b/src/frontends/qt4/GuiFontLoader.cpp
index cc092f5..45392d8 100644
--- a/src/frontends/qt4/GuiFontLoader.cpp
+++ b/src/frontends/qt4/GuiFontLoader.cpp
@@ -373,9 +373,13 @@ GuiFontInfo::GuiFontInfo(FontInfo const & f)
 
 bool FontLoader::available(FontInfo const & f)
 {
-	// FIXME THREAD
+#if defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6)
 	static vector cache_set(NUM_FAMILIES, false);
 	static vector cache(NUM_FAMILIES, false);
+#else
+	thread_local vector cache_set(NUM_FAMILIES, false);
+	thread_local vector cache(NUM_FAMILIES, false);
+#endif
 
 	FontFamily family = f.family();
 #ifdef Q_OS_MAC
diff --git a/src/frontends/qt4/GuiInclude.cpp b/src/frontends/qt4/GuiInclude.cpp
index e1fc275..2fe7f8f 100644
--- a/src/frontends/qt4/GuiInclude.cpp
+++ b/src/frontends/qt4/GuiInclude.cpp
@@ -93,10 +93,14 @@ docstring GuiInclude::validate_listings_params()
 {
 	// use a cache here to avoid repeated validation
 	// of the same parameters
-	// FIXME THREAD
-	static string param_cache = string();
-	static docstring msg_cache = docstring();
-	
+#if defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC