Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Hi Guillaume,

I thought about it again, and you are right, of course.

JMarc

Le 24 février 2017 22:10:17 GMT+01:00, Guillaume Munch  a écrit :
>Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :

>> It would be nice to hide these subtleties in the Cache
>implementation,
>> if possible.
>>
>I am not sure what you mean with "hide these subtleties". The
>specification is simple: if the key does not exist in the cache, you
>get
>the default object. 

Re: Memory leak in master?

[Guillaume Munch]

Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :

Le 21/02/2017 à 20:13, Guillaume Munch a écrit :


BTW, why don't you use Cache::contains in getLayout like you do for
other cache uses?



This is explained in the documentation of Cache::object. It is enough to
check for a null pointer in that case.


It would be nice to hide these subtleties in the Cache implementation,
if possible.



Hi Jean-Marc,

I am not sure what you mean with "hide these subtleties". The
specification is simple: if the key does not exist in the cache, you get
the default object. I thought about using boost::optional at first but
for the main use I had in mind (shared_ptr) I found that the code would
be heavier (you end up having to dereference twice) for little clarity gain.

Guillaume

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 23/02/2017 à 18:05, Richard Heck a écrit :


Here is for reference the updated patch (even with comment
clarification). Richard?


OK. I'll test it over the next couple weeks, and if all is well move
toward 2.2.3.

rh

Done at 998c3e7c8ef. There is no status.22x entry, since this is a fix 
over a new feature.


JMarc

Re: Memory leak in master?

[Richard Heck]

On 02/23/2017 04:46 AM, Jean-Marc Lasgouttes wrote:
> Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :
>> Indeed. Richard, I think this patch should go in 2.2.3, because the
>> caching code that is in stable causes bad memory leaks with Qt5.
>
> Here is for reference the updated patch (even with comment
> clarification). Richard?

OK. I'll test it over the next couple weeks, and if all is well move
toward 2.2.3.

rh

Re: Memory leak in master?

[Richard Heck]

On 02/23/2017 04:46 AM, Jean-Marc Lasgouttes wrote:
> Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :
>> Indeed. Richard, I think this patch should go in 2.2.3, because the
>> caching code that is in stable causes bad memory leaks with Qt5.
>
> Here is for reference the updated patch (even with comment
> clarification). Richard?

OK. I'll test it over the next couple weeks.

rh

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 22/02/2017 à 18:03, Jean-Marc Lasgouttes a écrit :

Indeed. Richard, I think this patch should go in 2.2.3, because the
caching code that is in stable causes bad memory leaks with Qt5.


Here is for reference the updated patch (even with comment 
clarification). Richard?


JMarc


From 8e04248b2fea58a7edcf16fb3318c302b04c66e8 Mon Sep 17 00:00:00 2001
From: Guillaume Munch 
Date: Mon, 20 Feb 2017 23:59:24 +0100
Subject: [PATCH] Introduce support/Cache.h

Useful to cache copies of objects, including shared_ptrs. No risks of dangling
pointer, and avoid naked pointers in the source.

Fix memory leak when compiling with Qt5.

As part as the backport to stable, this code has been change to work
with C++98.

(cherry picked from commit 33b696c8acf2e64b44d449180781de6dbc203709)
(cherry picked from commit e04079aa528ecbf4a8e39ed2b19c3cb50174e151)
(cherry picked from commit 5211ca52cac2ad7a6669d15c39f2cee172d18323)
(cherry picked from commit 8353a53cc38fe364bee516e86a08251e4ae974fc)
---
 src/frontends/qt4/GuiFontMetrics.cpp |  126 +++---
 src/frontends/qt4/GuiFontMetrics.h   |   46 +++--
 src/frontends/qt4/GuiPainter.cpp |3 +-
 src/support/Cache.h  |   89 
 src/support/Makefile.am  |1 +
 5 files changed, 160 insertions(+), 105 deletions(-)
 create mode 100644 src/support/Cache.h

diff --git a/src/frontends/qt4/GuiFontMetrics.cpp b/src/frontends/qt4/GuiFontMetrics.cpp
index 5bb99aa..80ff819 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -24,14 +24,11 @@
 #include "support/convert.h"
 #include "support/lassert.h"
 
-#ifdef CACHE_SOME_METRICS
 #include 
-#endif
 
 using namespace std;
 using namespace lyx::support;
 
-#ifdef CACHE_SOME_METRICS
 namespace std {
 
 /*
@@ -46,11 +43,28 @@ uint qHash(lyx::docstring const & s)
 }
 
 }
-#endif
 
 namespace lyx {
 namespace frontend {
 
+
+/*
+ * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
+ * Limit qtextlayout_cache_ size to 500 elements (we do not know the
+ * size of the QTextLayout objects anyway).
+ * Note that all these numbers are arbitrary.
+ * Also, setting size to 0 is tantamount to disabling the cache.
+ */
+int cache_metrics_width_size = 1 << 19;
+int cache_metrics_breakat_size = 1 << 19;
+// Qt 5.x already has its own caching of QTextLayout objects
+#if (QT_VERSION < 0x05)
+int cache_metrics_qtextlayout_size = 500;
+#else
+int cache_metrics_qtextlayout_size = 0;
+#endif
+
+
 namespace {
 /**
  * Convert a UCS4 character into a QChar.
@@ -73,23 +87,11 @@ inline QChar const ucs4_to_qchar(char_type const ucs4)
 } // anon namespace
 
 
-/*
- * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
- * Limit qtextlayout_cache_ size to 500 elements (we do not know the
- * size of the QTextLayout objects anyway).
- * Note that all these numbers are arbitrary.
- */
 GuiFontMetrics::GuiFontMetrics(QFont const & font)
-	: font_(font), metrics_(font, 0)
-#ifdef CACHE_METRICS_WIDTH
-	, strwidth_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_BREAKAT
-	, breakat_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	,  qtextlayout_cache_(500)
-#endif
+	: font_(font), metrics_(font, 0),
+	  strwidth_cache_(cache_metrics_width_size),
+	  breakat_cache_(cache_metrics_breakat_size),
+	  qtextlayout_cache_(cache_metrics_qtextlayout_size)
 {
 }
 
@@ -174,11 +176,8 @@ int GuiFontMetrics::rbearing(char_type c) const
 
 int GuiFontMetrics::width(docstring const & s) const
 {
-#ifdef CACHE_METRICS_WIDTH
-	int * pw = strwidth_cache_[s];
-	if (pw)
-		return *pw;
-#endif
+	if (strwidth_cache_.contains(s))
+		return strwidth_cache_[s];
 	/* For some reason QMetrics::width returns a wrong value with Qt5
 	 * with some arabic text. OTOH, QTextLayout is broken for single
 	 * characters with null width (like \not in mathed). Also, as a
@@ -200,9 +199,7 @@ int GuiFontMetrics::width(docstring const & s) const
 		tl.endLayout();
 		w = int(line.naturalTextWidth());
 	}
-#ifdef CACHE_METRICS_WIDTH
-	strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
-#endif
+	strwidth_cache_.insert(s, w, s.size() * sizeof(char_type));
 	return w;
 }
 
@@ -225,31 +222,26 @@ int GuiFontMetrics::signedWidth(docstring const & s) const
 }
 
 
-QTextLayout const *
+shared_ptr
 GuiFontMetrics::getTextLayout(docstring const & s, bool const rtl,
   double const wordspacing) const
 {
-	QTextLayout * ptl;
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	docstring const s_cache = s + (rtl ? "r" : "l") + convert(wordspacing);
-	ptl = qtextlayout_cache_[s_cache];
-	if (!ptl) {
-#endif
-		ptl = new QTextLayout();
-		ptl->setCacheEnabled(true);
-		ptl->setText(toqstr(s));
-		QFont copy = font_;
-		copy.setWordSpacing(wordspacing);
-		ptl->setFont(copy);
-		// Note that both setFlags and the enums are undocumented
-		ptl->setFlags(rtl ? Qt::TextForceRightToLeft : Qt::TextForceLeftToRight);
-		

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 21/02/2017 à 20:13, Guillaume Munch a écrit :

Thanks for doing this. I thought there was some bigger adaptation to the
code to do but indeed it only looks like a matter of C++98 conversion.


You mean it was a trap? It did not work %-p


I think it's all good except:

class Cache : private QCache {
-#if !(defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6))
+#if defined(LYX_USE_CXX11) && !(defined(__GNUC__) && (__GNUC__ == 4) &&
(__GNUC_MINOR__ == 6))


Indeed. Richard, I think this patch should go in 2.2.3, because the 
caching code that is in stable causes bad memory leaks with Qt5.



BTW, why don't you use Cache::contains in getLayout like you do for
other cache uses?



This is explained in the documentation of Cache::object. It is enough to
check for a null pointer in that case.


It would be nice to hide these subtleties in the Cache implementation, 
if possible.


JMarc

Re: Memory leak in master?

[Guillaume Munch]

Le 21/02/2017 à 07:19, Jean-Marc Lasgouttes a écrit :

Le 21/02/2017 à 00:08, Guillaume Munch a écrit :


Could you do the backport to stable? :)


What about that? Please check especially the code related to
LYX_USE_CXX11 in Cache.h. For the caching, I re-read the code to make
sure that my hand-merging was correct, I hope I did not miss anything.


Thanks for doing this. I thought there was some bigger adaptation to the
code to do but indeed it only looks like a matter of C++98 conversion.

I think it's all good except:

class Cache : private QCache {
-#if !(defined(__GNUC__) && (__GNUC__ == 4) && (__GNUC_MINOR__ == 6))
+#if defined(LYX_USE_CXX11) && !(defined(__GNUC__) && (__GNUC__ == 4) && 
(__GNUC_MINOR__ == 6))





BTW, why don't you use Cache::contains in getLayout like you do for
other cache uses?



This is explained in the documentation of Cache::object. It is enough to
check for a null pointer in that case.

Guillaume

Re: Memory leak in master?

[Guillaume Munch]

Le 20/02/2017 à 18:25, Jean-Marc Lasgouttes a écrit :

Le 10/02/2017 à 17:58, Guillaume Munch a écrit :

There's more data, but I am not sure how to interpret the results that
massif-visualizer shows.


If you send the file or make it available, I can take a look.


Here it is. But if it is like callgrind, it might lack the symbols.


Interestingly, the massif output that you sent also points to
MathMacro::clone(). Besides the obvious getTextLayout leak that needs to
be plugged, would you have an idea on why it is specifically macros that
appear here (with an increasing memory use, like the other)? I do not
see other math objects doing that.

Note that there are other sources that point to MathAtom::MathAtom, as
the drop in percentage shows.



Interesting, massif-visualizer shows the results differently. With it
one sees that MathAtom quickly reaches its final space usage and even
decreases at some point. It does not look similar to the slow creep of
the memory leak. Again, I am not sure that I know how to interpret the
results.

Guillaume

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 21/02/2017 à 00:08, Guillaume Munch a écrit :


Could you do the backport to stable? :)


What about that? Please check especially the code related to 
LYX_USE_CXX11 in Cache.h. For the caching, I re-read the code to make 
sure that my hand-merging was correct, I hope I did not miss anything.


BTW, why don't you use Cache::contains in getLayout like you do for 
other cache uses?


JMarc



From 30b62647ba2dc111aeb4425292e92994cf96f931 Mon Sep 17 00:00:00 2001
From: Guillaume Munch 
Date: Mon, 20 Feb 2017 23:59:24 +0100
Subject: [PATCH] Introduce support/Cache.h

Useful to cache copies of objects, including shared_ptrs. No risks of dangling
pointer, and avoid naked pointers in the source.

Fix memory leak when compiling with Qt5.

As part as the backport to stable, this code has been change to work
with C++98.

(cherry picked from commit 33b696c8acf2e64b44d449180781de6dbc203709)
(cherry picked from commit e04079aa528ecbf4a8e39ed2b19c3cb50174e151)
(cherry picked from commit 5211ca52cac2ad7a6669d15c39f2cee172d18323)
---
 src/frontends/qt4/GuiFontMetrics.cpp |  126 +++---
 src/frontends/qt4/GuiFontMetrics.h   |   46 +++--
 src/frontends/qt4/GuiPainter.cpp |3 +-
 src/support/Cache.h  |   88 
 src/support/Makefile.am  |1 +
 5 files changed, 159 insertions(+), 105 deletions(-)
 create mode 100644 src/support/Cache.h

diff --git a/src/frontends/qt4/GuiFontMetrics.cpp b/src/frontends/qt4/GuiFontMetrics.cpp
index 5bb99aa..80ff819 100644
--- a/src/frontends/qt4/GuiFontMetrics.cpp
+++ b/src/frontends/qt4/GuiFontMetrics.cpp
@@ -24,14 +24,11 @@
 #include "support/convert.h"
 #include "support/lassert.h"
 
-#ifdef CACHE_SOME_METRICS
 #include 
-#endif
 
 using namespace std;
 using namespace lyx::support;
 
-#ifdef CACHE_SOME_METRICS
 namespace std {
 
 /*
@@ -46,11 +43,28 @@ uint qHash(lyx::docstring const & s)
 }
 
 }
-#endif
 
 namespace lyx {
 namespace frontend {
 
+
+/*
+ * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
+ * Limit qtextlayout_cache_ size to 500 elements (we do not know the
+ * size of the QTextLayout objects anyway).
+ * Note that all these numbers are arbitrary.
+ * Also, setting size to 0 is tantamount to disabling the cache.
+ */
+int cache_metrics_width_size = 1 << 19;
+int cache_metrics_breakat_size = 1 << 19;
+// Qt 5.x already has its own caching of QTextLayout objects
+#if (QT_VERSION < 0x05)
+int cache_metrics_qtextlayout_size = 500;
+#else
+int cache_metrics_qtextlayout_size = 0;
+#endif
+
+
 namespace {
 /**
  * Convert a UCS4 character into a QChar.
@@ -73,23 +87,11 @@ inline QChar const ucs4_to_qchar(char_type const ucs4)
 } // anon namespace
 
 
-/*
- * Limit (strwidth|breakat)_cache_ size to 512kB of string data.
- * Limit qtextlayout_cache_ size to 500 elements (we do not know the
- * size of the QTextLayout objects anyway).
- * Note that all these numbers are arbitrary.
- */
 GuiFontMetrics::GuiFontMetrics(QFont const & font)
-	: font_(font), metrics_(font, 0)
-#ifdef CACHE_METRICS_WIDTH
-	, strwidth_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_BREAKAT
-	, breakat_cache_(1 << 19)
-#endif
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	,  qtextlayout_cache_(500)
-#endif
+	: font_(font), metrics_(font, 0),
+	  strwidth_cache_(cache_metrics_width_size),
+	  breakat_cache_(cache_metrics_breakat_size),
+	  qtextlayout_cache_(cache_metrics_qtextlayout_size)
 {
 }
 
@@ -174,11 +176,8 @@ int GuiFontMetrics::rbearing(char_type c) const
 
 int GuiFontMetrics::width(docstring const & s) const
 {
-#ifdef CACHE_METRICS_WIDTH
-	int * pw = strwidth_cache_[s];
-	if (pw)
-		return *pw;
-#endif
+	if (strwidth_cache_.contains(s))
+		return strwidth_cache_[s];
 	/* For some reason QMetrics::width returns a wrong value with Qt5
 	 * with some arabic text. OTOH, QTextLayout is broken for single
 	 * characters with null width (like \not in mathed). Also, as a
@@ -200,9 +199,7 @@ int GuiFontMetrics::width(docstring const & s) const
 		tl.endLayout();
 		w = int(line.naturalTextWidth());
 	}
-#ifdef CACHE_METRICS_WIDTH
-	strwidth_cache_.insert(s, new int(w), s.size() * sizeof(char_type));
-#endif
+	strwidth_cache_.insert(s, w, s.size() * sizeof(char_type));
 	return w;
 }
 
@@ -225,31 +222,26 @@ int GuiFontMetrics::signedWidth(docstring const & s) const
 }
 
 
-QTextLayout const *
+shared_ptr
 GuiFontMetrics::getTextLayout(docstring const & s, bool const rtl,
   double const wordspacing) const
 {
-	QTextLayout * ptl;
-#ifdef CACHE_METRICS_QTEXTLAYOUT
-	docstring const s_cache = s + (rtl ? "r" : "l") + convert(wordspacing);
-	ptl = qtextlayout_cache_[s_cache];
-	if (!ptl) {
-#endif
-		ptl = new QTextLayout();
-		ptl->setCacheEnabled(true);
-		ptl->setText(toqstr(s));
-		QFont copy = font_;
-		copy.setWordSpacing(wordspacing);
-		ptl->setFont(copy);
-		// Note that both setFlags and the enums are undocumented
-		ptl->setFlags(rtl ? 

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 21/02/2017 à 00:08, Guillaume Munch a écrit :

Le 20/02/2017 à 18:27, Jean-Marc Lasgouttes a écrit :

Le 13/02/2017 à 20:55, Jean-Marc Lasgouttes a écrit :

What I mean is that you can easily force
QCache into shared ownership by replacing

QCache

with

QCache

and create the appropriate wrappers for insertion and retrieval.


Could you have a go at this solution?



Could you do the backport to stable? :)


You mean the version that does not require C++11? Mmm, I can always have 
a go at it.


JMarc

Re: Memory leak in master?

[Guillaume Munch]

Le 20/02/2017 à 18:27, Jean-Marc Lasgouttes a écrit :

Le 13/02/2017 à 20:55, Jean-Marc Lasgouttes a écrit :

What I mean is that you can easily force
QCache into shared ownership by replacing

QCache

with

QCache

and create the appropriate wrappers for insertion and retrieval.


Could you have a go at this solution?



Could you do the backport to stable? :)

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 13/02/2017 à 20:55, Jean-Marc Lasgouttes a écrit :

What I mean is that you can easily force
QCache into shared ownership by replacing

QCache

with

QCache

and create the appropriate wrappers for insertion and retrieval.


Could you have a go at this solution?

JMarc

Re: Memory leak in master?

[Jean-Marc Lasgouttes]

Le 10/02/2017 à 17:58, Guillaume Munch a écrit :

There's more data, but I am not sure how to interpret the results that
massif-visualizer shows.


If you send the file or make it available, I can take a look.


Here it is. But if it is like callgrind, it might lack the symbols.


Interestingly, the massif output that you sent also points to 
MathMacro::clone(). Besides the obvious getTextLayout leak that needs to 
be plugged, would you have an idea on why it is specifically macros that 
appear here (with an increasing memory use, like the other)? I do not 
see other math objects doing that.


Note that there are other sources that point to MathAtom::MathAtom, as 
the drop in percentage shows.


JMarc

->09.75% (18,139,776B) 0x8DA8B5: 
lyx::MathMacro::MathMacro(lyx::MathMacro const&) (MathMacro.cpp:299)
| ->09.75% (18,139,776B) 0x8DB03F: lyx::MathMacro::clone() const 
(MathMacro.cpp:394)
|   ->09.75% (18,139,776B) 0x8AD26D: 
lyx::MathAtom::MathAtom(lyx::MathAtom const&) (MathAtom.cpp:35)
| ->03.42% (6,356,216B) 0x86ADFE: 
std::vector::operator=(std::vector const&) 
(stl_construct.h:75)
| | ->01.16% (2,158,056B) 0x8EF837: lyx::(anonymous 
namespace)::Parser::parse(lyx::MathData&, unsigned int, 
lyx::Inset::mode_type) (vector:111)
| | | ->01.16% (2,158,056B) 0x8EFC99: 
lyx::mathed_parse_cell(lyx::MathData&, lyx::docstring const&, 
lyx::Parse::flags) (MathParser.cpp:2150)
| | | | ->01.16% (2,158,056B) 0x9079EB: lyx::asArray(lyx::docstring 
const&, lyx::MathData&, lyx::Parse::flags) (MathSupport.cpp:985)
| | | |   ->01.16% (2,158,056B) 0x8DBBE8: 
lyx::MathMacro::updateRepresentation(lyx::Cursor*, lyx::MacroContext 
const&, lyx::UpdateType, int) (MathMacro.cpp:699)