From 7c614cc6b723bbdd5a65a5eb6dab8c400b2ec017 Mon Sep 17 00:00:00 2001 From: Richard Kimberly Heck Date: Sun, 2 Sep 2018 00:10:01 -0400 Subject: [PATCH] Try yet again try to fix bug #9158. The problem with the previous attempt was that, every time through updateBuffer, we looked up the file location using kpsewhich, which took too long on Windows. The new solution is to cache that info, and to look it up only when we need it. Previously, this info would have been re-read whenever we parsed the bibfiles. So we re-read it now whenever the bibinfo cache is invalid, which is less often, but should be good enough. We can add more such re-reads if need be. (cherry picked from commit 601449269939150bb7452698809e59c00a98a20f) --- src/Buffer.cpp | 132 ++++++++++++++++++++++++++---------- src/Buffer.h | 11 +-- src/BufferView.cpp | 5 +- src/insets/InsetBibtex.cpp | 56 +++------------ src/insets/InsetBibtex.h | 7 +- src/insets/InsetInclude.cpp | 7 -- src/support/FileNameList.h | 10 --- 7 files changed, 119 insertions(+), 109 deletions(-) diff --git a/src/Buffer.cpp b/src/Buffer.cpp index 71b26f9590..4ea890af2c 100644 --- a/src/Buffer.cpp +++ b/src/Buffer.cpp @@ -279,7 +279,7 @@ public: /// A cache for the bibfiles (including bibfiles of loaded child /// documents), needed for appropriate update of natbib labels. - mutable support::FileNamePairList bibfiles_cache_; + mutable docstring_list bibfiles_cache_; // FIXME The caching mechanism could be improved. At present, we have a // cache for each Buffer, that caches all the bibliography info for that @@ -344,7 +344,6 @@ public: LYXERR0("Warning: a buffer should not have two parents!"); parent_buffer = pb; if (!cloned_buffer_ && parent_buffer) { - parent_buffer->invalidateBibfileCache(); parent_buffer->invalidateBibinfoCache(); } } @@ -1904,7 +1903,7 @@ void Buffer::writeLaTeXSource(otexstream & os, // Biblatex bibliographies are loaded here if (params().useBiblatex()) { vector const bibfiles = - prepareBibFilePaths(runparams, getBibfilesCache(), true); + prepareBibFilePaths(runparams, getBibfiles(), true); for (docstring const & file: bibfiles) os << "\\addbibresource{" << file << "}\n"; } @@ -2335,7 +2334,7 @@ void Buffer::updateBibfilesCache(UpdateScope scope) const for (InsetIterator it = inset_iterator_begin(inset()); it; ++it) { if (it->lyxCode() == BIBTEX_CODE) { InsetBibtex const & inset = static_cast(*it); - support::FileNamePairList const bibfiles = inset.getBibFiles(); + docstring_list const bibfiles = inset.getBibFiles(); d->bibfiles_cache_.insert(d->bibfiles_cache_.end(), bibfiles.begin(), bibfiles.end()); @@ -2344,8 +2343,8 @@ void Buffer::updateBibfilesCache(UpdateScope scope) const Buffer const * const incbuf = inset.getChildBuffer(); if (!incbuf) continue; - support::FileNamePairList const & bibfiles = - incbuf->getBibfilesCache(UpdateChildOnly); + docstring_list const & bibfiles = + incbuf->getBibfiles(UpdateChildOnly); if (!bibfiles.empty()) { d->bibfiles_cache_.insert(d->bibfiles_cache_.end(), bibfiles.begin(), @@ -2370,25 +2369,13 @@ void Buffer::invalidateBibinfoCache() const } -void Buffer::invalidateBibfileCache() const -{ - d->bibfile_cache_valid_ = false; - d->bibinfo_cache_valid_ = false; - d->cite_labels_valid_ = false; - // also invalidate the cache for the parent buffer - Buffer const * const pbuf = d->parent(); - if (pbuf) - pbuf->invalidateBibfileCache(); -} - - -support::FileNamePairList const & Buffer::getBibfilesCache(UpdateScope scope) const +docstring_list const & Buffer::getBibfiles(UpdateScope scope) const { // FIXME This is probably unnecessary, given where we call this. // If this is a child document, use the master's cache instead. Buffer const * const pbuf = masterBuffer(); if (pbuf != this && scope != UpdateChildOnly) - return pbuf->getBibfilesCache(); + return pbuf->getBibfiles(); if (!d->bibfile_cache_valid_) this->updateBibfilesCache(scope); @@ -2412,6 +2399,49 @@ BiblioInfo const & Buffer::bibInfo() const } +void Buffer::registerBibfiles(const docstring_list & bf) const +{ + // We register the bib files in the master buffer, + // if there is one, but also in every single buffer, + // in case a child is compiled alone. + Buffer const * const tmp = masterBuffer(); + if (tmp != this) + tmp->registerBibfiles(bf); + + for (auto const & p : bf) { + docstring_list::const_iterator temp = + find(d->bibfiles_cache_.begin(), d->bibfiles_cache_.end(), p); + if (temp == d->bibfiles_cache_.end()) + d->bibfiles_cache_.push_back(p); + } +} + + +static map bibfileCache; + +FileName Buffer::getBibfilePath(docstring const & bibid) const +{ + map::const_iterator it = + bibfileCache.find(bibid); + if (it != bibfileCache.end()) { + // i.e., bibfileCache[bibid] + return it->second; + } + + LYXERR(Debug::FILES, "Reading file location for " << bibid); + string texfile = changeExtension(to_utf8(bibid), "bib"); + // note that, if the filename can be found directly from the path, + // findtexfile will just return a FileName object for that path. + FileName file(findtexfile(texfile, "bib")); + if (file.empty()) + file = FileName(makeAbsPath(texfile, filePath())); + LYXERR(Debug::FILES, "Found at: " << file); + + bibfileCache[bibid] = file; + return bibfileCache[bibid]; +} + + void Buffer::checkIfBibInfoCacheIsValid() const { // use the master's cache @@ -2422,11 +2452,9 @@ void Buffer::checkIfBibInfoCacheIsValid() const } // compare the cached timestamps with the actual ones. - FileNamePairList const & bibfiles_cache = getBibfilesCache(); - FileNamePairList::const_iterator ei = bibfiles_cache.begin(); - FileNamePairList::const_iterator en = bibfiles_cache.end(); - for (; ei != en; ++ ei) { - FileName const fn = ei->second; + docstring_list const & bibfiles_cache = getBibfiles(); + for (auto const & bf : bibfiles_cache) { + FileName const fn = getBibfilePath(bf); time_t lastw = fn.lastModified(); time_t prevw = d->bibfile_status_[fn]; if (lastw != prevw) { @@ -2451,6 +2479,11 @@ void Buffer::reloadBibInfoCache() const if (d->bibinfo_cache_valid_) return; + // re-read file locations when this info changes + // FIXME Is this sufficient? Or should we also force that + // in some other cases? If so, then it is easy enough to + // add the following line in some other places. + bibfileCache.clear(); d->bibinfo_.clear(); FileNameList checkedFiles; collectBibKeys(checkedFiles); @@ -3173,7 +3206,7 @@ string const Buffer::prepareFileNameForLaTeX(string const & name, vector const Buffer::prepareBibFilePaths(OutputParams const & runparams, - FileNamePairList const bibfilelist, + docstring_list const & bibfilelist, bool const add_extension) const { // If we are processing the LaTeX file in a temp directory then @@ -3194,10 +3227,8 @@ vector const Buffer::prepareBibFilePaths(OutputParams const & runpara // check for spaces in paths bool found_space = false; - FileNamePairList::const_iterator it = bibfilelist.begin(); - FileNamePairList::const_iterator en = bibfilelist.end(); - for (; it != en; ++it) { - string utf8input = to_utf8(it->first); + for (auto const & bit : bibfilelist) { + string utf8input = to_utf8(bit); string database = prepareFileNameForLaTeX(utf8input, ".bib", runparams.nice); FileName try_in_file = @@ -3206,7 +3237,7 @@ vector const Buffer::prepareBibFilePaths(OutputParams const & runpara // If the file has not been found, try with the real file name // (it might come from a child in a sub-directory) if (!not_from_texmf) { - try_in_file = it->second; + try_in_file = getBibfilePath(bit); if (try_in_file.isReadableFile()) { // Check if the file is in texmf FileName kpsefile(findtexfile(changeExtension(utf8input, "bib"), "bib", true)); @@ -4761,8 +4792,10 @@ void Buffer::updateBuffer(UpdateScope scope, UpdateType utype) const Buffer const * const master = masterBuffer(); DocumentClass const & textclass = master->params().documentClass(); - // do this only if we are the top-level Buffer - if (master == this) { + docstring_list old_bibfiles; + // Do this only if we are the top-level Buffer. We also need to account + // for the case of a previewed child with ignored parent here. + if (master == this && !d->ignore_parent) { textclass.counters().reset(from_ascii("bibitem")); reloadBibInfoCache(); } @@ -4817,7 +4850,38 @@ void Buffer::updateBuffer(UpdateScope scope, UpdateType utype) const // in InsetInclude::addToToc. return; - d->bibinfo_cache_valid_ = true; + // if the bibfiles changed, the cache of bibinfo is invalid + docstring_list new_bibfiles = d->bibfiles_cache_; + // this is a trick to determine whether the two vectors have + // the same elements. + sort(new_bibfiles.begin(), new_bibfiles.end()); + sort(old_bibfiles.begin(), old_bibfiles.end()); + if (old_bibfiles != new_bibfiles) { + LYXERR(Debug::FILES, "Reloading bibinfo cache."); + invalidateBibinfoCache(); + reloadBibInfoCache(); + // We relied upon the bibinfo cache when recalculating labels. But that + // cache was invalid, although we didn't find that out until now. So we + // have to do it all again. + // That said, the only thing we really need to do is update the citation + // labels. Nothing else will have changed. So we could create a new + // UpdateType that would signal that fact, if we needed to do so. + parit = cbuf.par_iterator_begin(); + // we will be re-doing the counters and references and such. + textclass.counters().reset(); + clearReferenceCache(); + // we should not need to do this again? + // updateMacros(); + setChangesPresent(false); + updateBuffer(parit, utype); + // this will already have been done by reloadBibInfoCache(); + // d->bibinfo_cache_valid_ = true; + } + else { + LYXERR(Debug::FILES, "Bibfiles unchanged."); + // this is also set to true on the other path, by reloadBibInfoCache. + d->bibinfo_cache_valid_ = true; + } d->cite_labels_valid_ = true; /// FIXME: Perf cbuf.tocBackend().update(true, utype); diff --git a/src/Buffer.h b/src/Buffer.h index a68ad046cf..b666b6610c 100644 --- a/src/Buffer.h +++ b/src/Buffer.h @@ -70,7 +70,6 @@ class WorkAreaManager; namespace support { class DocFileName; class FileName; -class FileNamePairList; } // namespace support namespace graphics { @@ -416,7 +415,7 @@ public: * output in the respective BibTeX/Biblatex macro */ std::vector const prepareBibFilePaths(OutputParams const &, - support::FileNamePairList const bibfilelist, + const docstring_list & bibfilelist, bool const extension = true) const; /** Returns the path where a local layout file lives. @@ -779,6 +778,10 @@ public: void setChangesPresent(bool) const; bool areChangesPresent() const; void updateChangesPresent() const; + /// + void registerBibfiles(docstring_list const & bf) const; + /// + support::FileName getBibfilePath(docstring const & bibid) const; private: friend class MarkAsExporting; @@ -800,8 +803,8 @@ private: void updateBibfilesCache(UpdateScope scope = UpdateMaster) const; /// Return the list with all bibfiles in use (including bibfiles /// of loaded child documents). - support::FileNamePairList const & - getBibfilesCache(UpdateScope scope = UpdateMaster) const; + docstring_list const & + getBibfiles(UpdateScope scope = UpdateMaster) const; /// void collectChildren(ListOfBuffers & children, bool grand_children) const; diff --git a/src/BufferView.cpp b/src/BufferView.cpp index 17dedce1b3..d2f324d168 100644 --- a/src/BufferView.cpp +++ b/src/BufferView.cpp @@ -1664,7 +1664,6 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) BIBTEX_CODE); if (inset) { if (inset->addDatabase(cmd.argument())) { - buffer_.invalidateBibfileCache(); dr.forceBufferUpdate(); } } @@ -1677,10 +1676,8 @@ void BufferView::dispatch(FuncRequest const & cmd, DispatchResult & dr) InsetBibtex * inset = getInsetByCode(tmpcur, BIBTEX_CODE); if (inset) { - if (inset->delDatabase(cmd.argument())) { - buffer_.invalidateBibfileCache(); + if (inset->delDatabase(cmd.argument())) dr.forceBufferUpdate(); - } } break; } diff --git a/src/insets/InsetBibtex.cpp b/src/insets/InsetBibtex.cpp index 2d164969e7..7a045932ba 100644 --- a/src/insets/InsetBibtex.cpp +++ b/src/insets/InsetBibtex.cpp @@ -37,6 +37,7 @@ #include "support/convert.h" #include "support/debug.h" #include "support/docstream.h" +#include "support/docstring_list.h" #include "support/ExceptionMessage.h" #include "support/FileNameList.h" #include "support/filetools.h" @@ -60,7 +61,6 @@ namespace os = support::os; InsetBibtex::InsetBibtex(Buffer * buf, InsetCommandParams const & p) : InsetCommand(buf, p) { - buffer().invalidateBibfileCache(); buffer().removeBiblioTempFiles(); } @@ -71,7 +71,6 @@ InsetBibtex::~InsetBibtex() /* We do not use buffer() because Coverity believes that this * may throw an exception. Actually this code path is not * taken when buffer_ == 0 */ - buffer_-> invalidateBibfileCache(); buffer_->removeBiblioTempFiles(); } } @@ -116,7 +115,6 @@ void InsetBibtex::doDispatch(Cursor & cur, FuncRequest & cmd) cur.recordUndo(); setParams(p); - buffer().invalidateBibfileCache(); buffer().removeBiblioTempFiles(); cur.forceBufferUpdate(); break; @@ -166,7 +164,7 @@ void InsetBibtex::editDatabases() const vector::const_iterator it = bibfilelist.begin(); vector::const_iterator en = bibfilelist.end(); for (; it != en; ++it) { - FileName const bibfile = getBibTeXPath(*it, buffer()); + FileName const bibfile = buffer().getBibfilePath(*it); theFormats().edit(buffer(), bibfile, theFormats().getFormatFromFile(bibfile)); } @@ -377,30 +375,9 @@ void InsetBibtex::latex(otexstream & os, OutputParams const & runparams) const } -support::FileNamePairList InsetBibtex::getBibFiles() const +docstring_list InsetBibtex::getBibFiles() const { - FileName path(buffer().filePath()); - support::PathChanger p(path); - - // We need to store both the real FileName and the way it is entered - // (with full path, rel path or as a single file name). - // The latter is needed for biblatex's central bibfile macro. - support::FileNamePairList vec; - - vector bibfilelist = getVectorFromString(getParam("bibfiles")); - vector::const_iterator it = bibfilelist.begin(); - vector::const_iterator en = bibfilelist.end(); - for (; it != en; ++it) { - FileName const file = getBibTeXPath(*it, buffer()); - - if (!file.empty()) - vec.push_back(make_pair(*it, file)); - else - LYXERR0("Couldn't find " + to_utf8(*it) + " in InsetBibtex::getBibFiles()!"); - } - - return vec; - + return getVectorFromString(getParam("bibfiles")); } namespace { @@ -673,11 +650,13 @@ void InsetBibtex::parseBibTeXFiles(FileNameList & checkedFiles) const BiblioInfo keylist; - support::FileNamePairList const files = getBibFiles(); - support::FileNamePairList::const_iterator it = files.begin(); - support::FileNamePairList::const_iterator en = files.end(); - for (; it != en; ++ it) { - FileName const bibfile = it->second; + docstring_list const files = getBibFiles(); + for (auto const & bf : files) { + FileName const bibfile = buffer().getBibfilePath(bf); + if (bibfile.empty()) { + LYXERR0("Unable to find path for " << bf << "!"); + continue; + } if (find(checkedFiles.begin(), checkedFiles.end(), bibfile) != checkedFiles.end()) // already checked this one. Skip. continue; @@ -691,7 +670,6 @@ void InsetBibtex::parseBibTeXFiles(FileNameList & checkedFiles) const VarMap strings; while (ifs) { - ifs.get(ch); if (!ifs) break; @@ -850,18 +828,6 @@ void InsetBibtex::parseBibTeXFiles(FileNameList & checkedFiles) const } -FileName InsetBibtex::getBibTeXPath(docstring const & filename, Buffer const & buf) -{ - string texfile = changeExtension(to_utf8(filename), "bib"); - // note that, if the filename can be found directly from the path, - // findtexfile will just return a FileName object for that path. - FileName file(findtexfile(texfile, "bib")); - if (file.empty()) - file = FileName(makeAbsPath(texfile, buf.filePath())); - return file; -} - - bool InsetBibtex::addDatabase(docstring const & db) { docstring bibfiles = getParam("bibfiles"); diff --git a/src/insets/InsetBibtex.h b/src/insets/InsetBibtex.h index 2d033464f0..ec783d857b 100644 --- a/src/insets/InsetBibtex.h +++ b/src/insets/InsetBibtex.h @@ -17,10 +17,10 @@ namespace lyx { class BiblioInfo; +class docstring_list; namespace support { class FileName; - class FileNamePairList; } // namespace support /** Used to insert BibTeX's information @@ -33,7 +33,7 @@ public: ~InsetBibtex(); /// - support::FileNamePairList getBibFiles() const; + docstring_list getBibFiles() const; /// bool addDatabase(docstring const &); /// @@ -78,9 +78,6 @@ public: //@} private: - /// look up the path to the file using TeX - static support::FileName - getBibTeXPath(docstring const & filename, Buffer const & buf); /// void editDatabases() const; /// diff --git a/src/insets/InsetInclude.cpp b/src/insets/InsetInclude.cpp index 7b6f38950f..89f2533111 100644 --- a/src/insets/InsetInclude.cpp +++ b/src/insets/InsetInclude.cpp @@ -197,11 +197,6 @@ InsetInclude::InsetInclude(InsetInclude const & other) InsetInclude::~InsetInclude() { - if (isBufferLoaded()) - /* We do not use buffer() because Coverity believes that this - * may throw an exception. Actually this code path is not - * taken when buffer_ == 0 */ - buffer_->invalidateBibfileCache(); delete label_; } @@ -354,8 +349,6 @@ void InsetInclude::setParams(InsetCommandParams const & p) if (type(params()) == INPUT) add_preview(*preview_, *this, buffer()); - - buffer().invalidateBibfileCache(); } diff --git a/src/support/FileNameList.h b/src/support/FileNameList.h index 894087d881..20b6a85af7 100644 --- a/src/support/FileNameList.h +++ b/src/support/FileNameList.h @@ -28,16 +28,6 @@ class FileNameList: public std::vector { }; - -/** - * List of filename with additional information. Used by the Bibfiles cache, - * which needs to store, next to the real filename, the way it was entered - * in the BibTeX inset (as abspath, relpath or texmf file) - */ -class FileNamePairList: public std::vector> -{ -}; - } // namespace support } // namespace lyx