From ead697d4b6c7122a2144b96712e6d2a3184f6fd8 Mon Sep 17 00:00:00 2001 From: Richard Heck Date: Tue, 13 Mar 2012 12:13:31 -0400 Subject: [PATCH] Deal with memory issue reported some time ago in connection with DocumentClass objects. The problem that led to the leak is that these objects can be held in memory long after the Buffer that created them is gone, mostly due to their use in the CutStack. So they were previously held in a storage facility, the DocumentClassBundle. Unfortunately, they were now being created too often, especially by cloning. It's not really a leak, because they're accessible, but we weren't ever destroying them. This new approach uses a shared_ptr instead. Thanks to Vincent for pointing out const_pointer_cast. --- src/BufferParams.cpp | 10 +++++----- src/BufferParams.h | 7 ++++--- src/BufferView.cpp | 14 ++++++-------- src/BufferView.h | 4 +++- src/CutAndPaste.cpp | 29 +++++++++++++--------------- src/CutAndPaste.h | 8 +++++--- src/DocumentClassPtr.h | 24 +++++++++++++++++++++++ src/Makefile.am | 1 + src/TextClass.cpp | 35 ++++------------------------------ src/TextClass.h | 41 +++++++++------------------------------- src/support/shared_ptr.h | 2 ++ src/tex2lyx/tex2lyx.cpp | 7 +++---- 12 files changed, 79 insertions(+), 103 deletions(-) create mode 100644 src/DocumentClassPtr.h diff --git a/src/BufferParams.cpp b/src/BufferParams.cpp index 7fdba576e7..817403f0de 100644 --- a/src/BufferParams.cpp +++ b/src/BufferParams.cpp @@ -1953,20 +1953,20 @@ bool BufferParams::hasClassDefaults() const DocumentClass const & BufferParams::documentClass() const { - return *doc_class_; + return *doc_class_.get(); } -DocumentClass const * BufferParams::documentClassPtr() const +DocumentClassConstPtr BufferParams::documentClassPtr() const { return doc_class_; } -void BufferParams::setDocumentClass(DocumentClass const * const tc) +void BufferParams::setDocumentClass(DocumentClassConstPtr tc) { // evil, but this function is evil - doc_class_ = const_cast(tc); + doc_class_ = const_pointer_cast(tc); } @@ -2037,7 +2037,7 @@ void BufferParams::makeDocumentClass() en = cite_engine_.end(); for (; it != en; ++it) mods.push_back(*it); - doc_class_ = &(DocumentClassBundle::get().makeDocumentClass(*baseClass(), mods)); + doc_class_ = getDocumentClass(*baseClass(), mods); if (!local_layout.empty()) { TextClass::ReturnValues success = diff --git a/src/BufferParams.h b/src/BufferParams.h index 772f42de84..f4f9bef8e0 100644 --- a/src/BufferParams.h +++ b/src/BufferParams.h @@ -16,6 +16,7 @@ #define BUFFERPARAMS_H #include "Citation.h" +#include "DocumentClassPtr.h" #include "Format.h" #include "LayoutModuleList.h" #include "OutputParams.h" @@ -129,11 +130,11 @@ public: DocumentClass const & documentClass() const; /// \return A pointer to the DocumentClass currently in use: the BaseClass /// as modified by modules. - DocumentClass const * documentClassPtr() const; + DocumentClassConstPtr documentClassPtr() const; /// This bypasses the baseClass and sets the textClass directly. /// Should be called with care and would be better not being here, /// but it seems to be needed by CutAndPaste::putClipboard(). - void setDocumentClass(DocumentClass const * const); + void setDocumentClass(DocumentClassConstPtr); /// List of modules in use LayoutModuleList const & getModules() const { return layout_modules_; } /// List of default modules the user has removed @@ -500,7 +501,7 @@ private: /// the type of cite engine (authoryear or numerical) CiteEngineType cite_engine_type_; /// - DocumentClass * doc_class_; + DocumentClassPtr doc_class_; /// LayoutModuleList layout_modules_; /// this is for modules that are required by the document class but that diff --git a/src/BufferView.cpp b/src/BufferView.cpp index 9b6c6b3c18..6c9aa366b8 100644 --- a/src/BufferView.cpp +++ b/src/BufferView.cpp @@ -941,7 +941,7 @@ bool BufferView::scrollToCursor(DocIterator const & dit, bool recenter) } -void BufferView::updateDocumentClass(DocumentClass const * const olddc) +void BufferView::updateDocumentClass(DocumentClassConstPtr olddc) { message(_("Converting document to new document class...")); @@ -1234,7 +1234,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) switch (act) { case LFUN_BUFFER_PARAMS_APPLY: { - DocumentClass const * const oldClass = buffer_.params().documentClassPtr(); + DocumentClassConstPtr oldClass = buffer_.params().documentClassPtr(); cur.recordUndoFullDocument(); istringstream ss(to_utf8(cmd.argument())); Lexer lex; @@ -1256,8 +1256,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) } case LFUN_LAYOUT_MODULES_CLEAR: { - DocumentClass const * const oldClass = - buffer_.params().documentClassPtr(); + DocumentClassConstPtr oldClass = buffer_.params().documentClassPtr(); cur.recordUndoFullDocument(); buffer_.params().clearLayoutModules(); buffer_.params().makeDocumentClass(); @@ -1275,7 +1274,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) "conflicts with installed modules."); break; } - DocumentClass const * const oldClass = params.documentClassPtr(); + DocumentClassConstPtr oldClass = params.documentClassPtr(); cur.recordUndoFullDocument(); buffer_.params().addLayoutModule(argument); buffer_.params().makeDocumentClass(); @@ -1306,8 +1305,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) break; // Save the old, possibly modular, layout for use in conversion. - DocumentClass const * const oldDocClass = - buffer_.params().documentClassPtr(); + DocumentClassConstPtr oldDocClass = buffer_.params().documentClassPtr(); cur.recordUndoFullDocument(); buffer_.params().setBaseClass(argument); buffer_.params().makeDocumentClass(); @@ -1332,7 +1330,7 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) } case LFUN_LAYOUT_RELOAD: { - DocumentClass const * const oldClass = buffer_.params().documentClassPtr(); + DocumentClassConstPtr oldClass = buffer_.params().documentClassPtr(); LayoutFileIndex bc = buffer_.params().baseClassID(); LayoutFileList::get().reset(bc); buffer_.params().setBaseClass(bc); diff --git a/src/BufferView.h b/src/BufferView.h index f6d5b1894b..d164438ad5 100644 --- a/src/BufferView.h +++ b/src/BufferView.h @@ -15,8 +15,10 @@ #ifndef BUFFER_VIEW_H #define BUFFER_VIEW_H +#include "DocumentClassPtr.h" #include "update_flags.h" +#include "support/shared_ptr.h" #include "support/strfwd.h" #include "support/types.h" @@ -345,7 +347,7 @@ private: void updateHoveredInset() const; /// - void updateDocumentClass(DocumentClass const * const olddc); + void updateDocumentClass(DocumentClassConstPtr olddc); /// int width_; /// diff --git a/src/CutAndPaste.cpp b/src/CutAndPaste.cpp index 757e318b6e..b84e52fc52 100644 --- a/src/CutAndPaste.cpp +++ b/src/CutAndPaste.cpp @@ -79,7 +79,7 @@ namespace { typedef pair PitPosPair; -typedef limited_stack > CutStack; +typedef limited_stack > CutStack; CutStack theCuts(10); // persistent selection, cleared until the next selection @@ -99,7 +99,7 @@ bool checkPastePossible(int index) pair pasteSelectionHelper(Cursor const & cur, ParagraphList const & parlist, - DocumentClass const * const oldDocClass, ErrorList & errorlist) + DocumentClassConstPtr oldDocClass, ErrorList & errorlist) { Buffer const & buffer = *cur.buffer(); pit_type pit = cur.pit(); @@ -119,8 +119,7 @@ pasteSelectionHelper(Cursor const & cur, ParagraphList const & parlist, // Make a copy of the CaP paragraphs. ParagraphList insertion = parlist; - DocumentClass const * const newDocClass = - buffer.params().documentClassPtr(); + DocumentClassConstPtr newDocClass = buffer.params().documentClassPtr(); // Now remove all out of the pars which is NOT allowed in the // new environment and set also another font if that is required. @@ -461,15 +460,14 @@ PitPosPair eraseSelectionHelper(BufferParams const & params, void putClipboard(ParagraphList const & paragraphs, - DocumentClass const * const docclass, docstring const & plaintext) + DocumentClassConstPtr docclass, docstring const & plaintext) { // For some strange reason gcc 3.2 and 3.3 do not accept // Buffer buffer(string(), false); - // This needs to be static to avoid a memory leak. When a Buffer is - // constructed, it constructs a BufferParams, which in turn constructs - // a DocumentClass, via new, that is never deleted. If we were to go to - // some kind of garbage collection there, or a shared_ptr, then this - // would not be needed. + // This used to need to be static to avoid a memory leak. It no longer needs + // to be so, but the alternative is to construct a new one of these (with a + // new temporary directory, etc) every time, and then to destroy it. So maybe + // it's worth just keeping this one around. static Buffer * buffer = theBufferList().newInternalBuffer( FileName::tempName("clipboard.internal").absFileName()); buffer->setUnnamed(true); @@ -503,7 +501,7 @@ static bool isFullyDeleted(ParagraphList const & pars) void copySelectionHelper(Buffer const & buf, Text const & text, pit_type startpit, pit_type endpit, - int start, int end, DocumentClass const * const dc, CutStack & cutstack) + int start, int end, DocumentClassConstPtr dc, CutStack & cutstack) { ParagraphList const & pars = text.paragraphs(); @@ -558,8 +556,7 @@ void copySelectionHelper(Buffer const & buf, Text const & text, it->setInsetOwner(0); } - DocumentClass * d = const_cast(dc); - cutstack.push(make_pair(copy_pars, d)); + cutstack.push(make_pair(copy_pars, dc)); } } // namespace anon @@ -634,8 +631,8 @@ bool multipleCellsSelected(Cursor const & cur) } -void switchBetweenClasses(DocumentClass const * const oldone, - DocumentClass const * const newone, InsetText & in, ErrorList & errorlist) +void switchBetweenClasses(DocumentClassConstPtr oldone, + DocumentClassConstPtr newone, InsetText & in, ErrorList & errorlist) { errorlist.clear(); @@ -975,7 +972,7 @@ docstring selection(size_t sel_index) void pasteParagraphList(Cursor & cur, ParagraphList const & parlist, - DocumentClass const * const docclass, ErrorList & errorList) + DocumentClassConstPtr docclass, ErrorList & errorList) { if (cur.inTexted()) { Text * text = cur.text(); diff --git a/src/CutAndPaste.h b/src/CutAndPaste.h index c5c7aa4334..2453601157 100644 --- a/src/CutAndPaste.h +++ b/src/CutAndPaste.h @@ -14,6 +14,8 @@ #ifndef CUTANDPASTE_H #define CUTANDPASTE_H +#include "DocumentClassPtr.h" + #include "support/docstring.h" #include "frontends/Clipboard.h" @@ -100,15 +102,15 @@ void pasteSimpleText(Cursor & cur, bool asParagraphs); /// Paste the paragraph list \p parlist at the position given by \p cur. /// Does not handle undo. Does only work in text, not mathed. void pasteParagraphList(Cursor & cur, ParagraphList const & parlist, - DocumentClass const * const textclass, ErrorList & errorList); + DocumentClassConstPtr textclass, ErrorList & errorList); /** Needed to switch between different classes. This works * for a list of paragraphs beginning with the specified par. * It changes layouts and character styles. */ -void switchBetweenClasses(DocumentClass const * const c1, - DocumentClass const * const c2, InsetText & in, ErrorList &); +void switchBetweenClasses(DocumentClassConstPtr c1, + DocumentClassConstPtr c2, InsetText & in, ErrorList &); /// Get the current selection as a string. Does not change the selection. /// Does only work if the whole selection is in mathed. diff --git a/src/DocumentClassPtr.h b/src/DocumentClassPtr.h new file mode 100644 index 0000000000..eef9ac2675 --- /dev/null +++ b/src/DocumentClassPtr.h @@ -0,0 +1,24 @@ +// -*- C++ -*- +/** + * \file DocumentClassPtr.h + * This file is part of LyX, the document processor. + * Licence details can be found in the file COPYING. + * + * \author Richard Heck + * + * Full author contact details are available in file CREDITS. + */ + +#ifndef DOCUMENT_CLASS_PTR_H +#define DOCUMENT_CLASS_PTR_H + +#include "support/shared_ptr.h" + +namespace lyx { +class DocumentClass; + +typedef shared_ptr DocumentClassPtr; +typedef shared_ptr DocumentClassConstPtr; +} + +#endif // DISPATCH_RESULT_H diff --git a/src/Makefile.am b/src/Makefile.am index bfd510ce86..02a878d56f 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -216,6 +216,7 @@ HEADERFILESCORE = \ DepTable.h \ DispatchResult.h \ DocIterator.h \ + DocumentClassPtr.h \ Encoding.h \ ErrorList.h \ Exporter.h \ diff --git a/src/TextClass.cpp b/src/TextClass.cpp index 65d5ef84d4..17e4f09c48 100644 --- a/src/TextClass.cpp +++ b/src/TextClass.cpp @@ -1431,38 +1431,11 @@ Layout TextClass::createBasicLayout(docstring const & name, bool unknown) const } -///////////////////////////////////////////////////////////////////////// -// -// DocumentClassBundle -// -///////////////////////////////////////////////////////////////////////// - -DocumentClassBundle::~DocumentClassBundle() -{ - for (size_t i = 0; i != documentClasses_.size(); ++i) - delete documentClasses_[i]; - documentClasses_.clear(); -} - -DocumentClass & DocumentClassBundle::newClass(LayoutFile const & baseClass) -{ - DocumentClass * dc = new DocumentClass(baseClass); - documentClasses_.push_back(dc); - return *documentClasses_.back(); -} - - -DocumentClassBundle & DocumentClassBundle::get() -{ - static DocumentClassBundle singleton; - return singleton; -} - - -DocumentClass & DocumentClassBundle::makeDocumentClass( +DocumentClassPtr getDocumentClass( LayoutFile const & baseClass, LayoutModuleList const & modlist) { - DocumentClass & doc_class = newClass(baseClass); + DocumentClassPtr doc_class = + DocumentClassPtr(new DocumentClass(baseClass)); LayoutModuleList::const_iterator it = modlist.begin(); LayoutModuleList::const_iterator en = modlist.end(); for (; it != en; ++it) { @@ -1490,7 +1463,7 @@ DocumentClass & DocumentClassBundle::makeDocumentClass( frontend::Alert::warning(_("Package not available"), msg, true); } FileName layout_file = libFileSearch("layouts", lm->getFilename()); - if (!doc_class.read(layout_file, TextClass::MODULE)) { + if (!doc_class->read(layout_file, TextClass::MODULE)) { docstring const msg = bformat(_("Error reading module %1$s\n"), from_utf8(modName)); frontend::Alert::warning(_("Read Error"), msg); diff --git a/src/TextClass.h b/src/TextClass.h index a1de81a516..fd8011328b 100644 --- a/src/TextClass.h +++ b/src/TextClass.h @@ -13,6 +13,7 @@ #include "Citation.h" #include "ColorCode.h" #include "Counters.h" +#include "DocumentClassPtr.h" #include "FloatList.h" #include "FontInfo.h" #include "Layout.h" @@ -24,8 +25,6 @@ #include "support/docstring.h" #include "support/types.h" -#include - #include #include #include @@ -369,7 +368,7 @@ private: /// In the main LyX code, DocumentClass objects are created only by /// DocumentClassBundle, for which see below. /// -class DocumentClass : public TextClass, boost::noncopyable { +class DocumentClass : public TextClass { public: /// virtual ~DocumentClass() {} @@ -475,41 +474,19 @@ protected: private: /// The only class that can create a DocumentClass is /// DocumentClassBundle, which calls the protected constructor. - friend class DocumentClassBundle; + friend DocumentClassPtr + getDocumentClass(LayoutFile const &, LayoutModuleList const &); /// static InsetLayout plain_insetlayout_; }; -/// DocumentClassBundle is a container for DocumentClass objects, so that -/// they stay in memory for use by Insets, CutAndPaste, and the like, even -/// when their associated Buffers are destroyed. -/// FIXME Some sort of garbage collection or reference counting wouldn't -/// be a bad idea here. It might be enough to check when a Buffer is closed -/// (or makeDocumentClass is called) whether the old DocumentClass is in use -/// anywhere. -/// -/// This is a singleton class. Its sole instance is accessed via -/// DocumentClassBundle::get(). -class DocumentClassBundle : boost::noncopyable { -public: - /// \return The sole instance of this class. - static DocumentClassBundle & get(); - /// \return A new DocumentClass based on baseClass, with info added - /// from the modules in modlist. - DocumentClass & makeDocumentClass(LayoutFile const & baseClass, +/// The only way to make a DocumentClass is to call this function. +/// The shared_ptr is needed because DocumentClass objects can be kept +/// in memory long after their associated Buffer is destroyed, mostly +/// on the CutStack. +DocumentClassPtr getDocumentClass(LayoutFile const & baseClass, LayoutModuleList const & modlist); -private: - /// control instantiation - DocumentClassBundle() {} - /// clean up - ~DocumentClassBundle(); - /// \return Reference to a new DocumentClass equal to baseClass - DocumentClass & newClass(LayoutFile const & baseClass); - /// - std::vector documentClasses_; -}; - /// convert page sides option to text 1 or 2 std::ostream & operator<<(std::ostream & os, PageSides p); diff --git a/src/support/shared_ptr.h b/src/support/shared_ptr.h index 28ac3d7a23..69e42da23c 100644 --- a/src/support/shared_ptr.h +++ b/src/support/shared_ptr.h @@ -23,6 +23,7 @@ namespace lyx { using std::tr1::shared_ptr; + using std::tr1::const_pointer_cast; } #else @@ -32,6 +33,7 @@ namespace lyx namespace lyx { using boost::shared_ptr; + using boost::const_pointer_cast; } #endif diff --git a/src/tex2lyx/tex2lyx.cpp b/src/tex2lyx/tex2lyx.cpp index 94e099a66c..d890130b6b 100644 --- a/src/tex2lyx/tex2lyx.cpp +++ b/src/tex2lyx/tex2lyx.cpp @@ -254,12 +254,11 @@ bool checkModule(string const & name, bool command) // This is needed since a module cannot be read on its own, only as // part of a document class. LayoutFile const & baseClass = LayoutFileList::get()[textclass.name()]; - typedef map ModuleMap; + typedef map ModuleMap; static ModuleMap modules; static bool init = true; if (init) { baseClass.load(); - DocumentClassBundle & bundle = DocumentClassBundle::get(); LyXModuleList::const_iterator const end = theModuleList.end(); LyXModuleList::const_iterator it = theModuleList.begin(); for (; it != end; ++it) { @@ -269,7 +268,7 @@ bool checkModule(string const & name, bool command) if (!m.moduleCanBeAdded(module, &baseClass)) continue; m.push_back(module); - modules[module] = &bundle.makeDocumentClass(baseClass, m); + modules[module] = getDocumentClass(baseClass, m); } init = false; } @@ -290,7 +289,7 @@ bool checkModule(string const & name, bool command) continue; if (findInsetLayoutWithoutModule(textclass, name, command)) continue; - DocumentClass const * c = it->second; + DocumentClassConstPtr c = it->second; Layout const * layout = findLayoutWithoutModule(*c, name, command); InsetLayout const * insetlayout = layout ? 0 : findInsetLayoutWithoutModule(*c, name, command); -- 2.39.2