Patches
Yuriy Skalko
yuriy.skalko at gmail.com
Thu Nov 5 10:43:48 UTC 2020
> Yes, updateMacros is a pain, it is actually a O(n^2) algorithm which kills performance for big documents. But it is a bit scary and I never dared changing it :)
>
> JMarc
Yes, macros implementation is really tangled. Maybe at first we can try
to minimize calling updateMacros?
I see that you've tried this, but reverted in 9e7832915f. What were the
problems due to this removal?
My profiling results are in attached SVG. I've done scrolling through
some of the manuals during profiled run.
Also attaching several patches with some improvements.
Yuriy
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graphviz.svg
Type: image/svg+xml
Size: 215826 bytes
Desc: not available
URL: <http://lists.lyx.org/pipermail/lyx-devel/attachments/20201105/54185018/attachment-0001.svg>
-------------- next part --------------
From bde33f98b606aa23e0fb21454028a9423c992961 Mon Sep 17 00:00:00 2001
From: Yuriy Skalko <yuriy.skalko at gmail.com>
Date: Wed, 4 Nov 2020 11:27:08 +0200
Subject: [PATCH 1/4] Improve structure of updateMacros
---
src/Buffer.cpp | 79 +++++++++++++++++++++--------------------
src/mathed/MathData.cpp | 4 +--
2 files changed, 42 insertions(+), 41 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index 95f2570484..b228d92392 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -3731,10 +3731,11 @@ void Buffer::Impl::updateMacros(DocIterator & it, DocIterator & scope)
for (auto const & insit : par.insetList()) {
it.pos() = insit.pos;
+ switch (insit.inset->lyxCode()) {
// is it a nested text inset?
- if (insit.inset->asInsetText()) {
- // Inset needs its own scope?
+ case TEXT_CODE: {
InsetText const * itext = insit.inset->asInsetText();
+ // Inset needs its own scope?
bool newScope = itext->isMacroScope();
// scope which ends just behind the inset
@@ -3745,10 +3746,9 @@ void Buffer::Impl::updateMacros(DocIterator & it, DocIterator & scope)
it.push_back(CursorSlice(*insit.inset));
updateMacros(it, newScope ? insetScope : scope);
it.pop_back();
- continue;
+ break;
}
-
- if (insit.inset->asInsetTabular()) {
+ case TABULAR_CODE: {
CursorSlice slice(*insit.inset);
size_t const numcells = slice.nargs();
for (; slice.idx() < numcells; slice.forwardIdx()) {
@@ -3756,11 +3756,10 @@ void Buffer::Impl::updateMacros(DocIterator & it, DocIterator & scope)
updateMacros(it, scope);
it.pop_back();
}
- continue;
+ break;
}
-
// is it an external file?
- if (insit.inset->lyxCode() == INCLUDE_CODE) {
+ case INCLUDE_CODE: {
// get buffer of external file
InsetInclude const & incinset =
static_cast<InsetInclude const &>(*insit.inset);
@@ -3768,7 +3767,7 @@ void Buffer::Impl::updateMacros(DocIterator & it, DocIterator & scope)
Buffer * child = incinset.loadIfNeeded();
macro_lock = false;
if (!child)
- continue;
+ break;
// register its position, but only when it is
// included first in the buffer
@@ -3776,38 +3775,40 @@ void Buffer::Impl::updateMacros(DocIterator & it, DocIterator & scope)
// register child with its scope
position_to_children[it] = Impl::ScopeBuffer(scope, child);
- continue;
+ break;
}
-
- InsetMath * im = insit.inset->asInsetMath();
- if (doing_export && im) {
- InsetMathHull * hull = im->asHullInset();
- if (hull)
- hull->recordLocation(it);
+ case MATH_HULL_CODE: {
+ if (!doing_export)
+ break;
+ InsetMathHull * hull = insit.inset->asInsetMath()->asHullInset();
+ hull->recordLocation(it);
+ break;
+ }
+ case MATHMACRO_CODE: {
+ // get macro data
+ InsetMathMacroTemplate & macroTemplate =
+ *insit.inset->asInsetMath()->asMacroTemplate();
+ MacroContext mc(owner_, it);
+ macroTemplate.updateToContext(mc);
+
+ // valid?
+ bool valid = macroTemplate.validMacro();
+ // FIXME: Should be fixNameAndCheckIfValid() in fact,
+ // then the BufferView's cursor will be invalid in
+ // some cases which leads to crashes.
+ if (!valid)
+ break;
+
+ // register macro
+ // FIXME (Abdel), I don't understand why we pass 'it' here
+ // instead of 'macroTemplate' defined above... is this correct?
+ macros[macroTemplate.name()][it] =
+ Impl::ScopeMacro(scope, MacroData(const_cast<Buffer *>(owner_), it));
+ break;
+ }
+ default:
+ break;
}
-
- if (insit.inset->lyxCode() != MATHMACRO_CODE)
- continue;
-
- // get macro data
- InsetMathMacroTemplate & macroTemplate =
- *insit.inset->asInsetMath()->asMacroTemplate();
- MacroContext mc(owner_, it);
- macroTemplate.updateToContext(mc);
-
- // valid?
- bool valid = macroTemplate.validMacro();
- // FIXME: Should be fixNameAndCheckIfValid() in fact,
- // then the BufferView's cursor will be invalid in
- // some cases which leads to crashes.
- if (!valid)
- continue;
-
- // register macro
- // FIXME (Abdel), I don't understand why we pass 'it' here
- // instead of 'macroTemplate' defined above... is this correct?
- macros[macroTemplate.name()][it] =
- Impl::ScopeMacro(scope, MacroData(const_cast<Buffer *>(owner_), it));
}
// next paragraph
diff --git a/src/mathed/MathData.cpp b/src/mathed/MathData.cpp
index 39d5f80184..ce1f4b5edc 100644
--- a/src/mathed/MathData.cpp
+++ b/src/mathed/MathData.cpp
@@ -403,8 +403,8 @@ void MathData::updateMacros(Cursor * cur, MacroContext const & mc,
{
// If we are editing a macro, we cannot update it immediately,
// otherwise wrong undo steps will be recorded (bug 6208).
- InsetMath const * inmath = cur ? cur->inset().asInsetMath() : 0;
- InsetMathMacro const * inmacro = inmath ? inmath->asMacro() : 0;
+ InsetMath const * inmath = cur ? cur->inset().asInsetMath() : nullptr;
+ InsetMathMacro const * inmacro = inmath ? inmath->asMacro() : nullptr;
docstring const edited_name = inmacro ? inmacro->name() : docstring();
// go over the array and look for macros
--
2.28.0.windows.1
-------------- next part --------------
From c88d539b8fb6a12b1aaa2d7c237502b55ca22323 Mon Sep 17 00:00:00 2001
From: Yuriy Skalko <yuriy.skalko at gmail.com>
Date: Wed, 4 Nov 2020 11:27:16 +0200
Subject: [PATCH 2/4] Remove redundant updateMacros call
---
src/CutAndPaste.cpp | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/CutAndPaste.cpp b/src/CutAndPaste.cpp
index ee3418dd65..6500b804b0 100644
--- a/src/CutAndPaste.cpp
+++ b/src/CutAndPaste.cpp
@@ -620,7 +620,6 @@ void putClipboard(ParagraphList const & paragraphs,
MarkAsExporting mex(buffer);
buffer->updateBuffer(Buffer::UpdateMaster, OutputUpdate);
- buffer->updateMacros();
buffer->updateMacroInstances(OutputUpdate);
// LyX's own format
--
2.28.0.windows.1
-------------- next part --------------
From 37f9c1b724a8262135b7e2971ea13eb0739bced7 Mon Sep 17 00:00:00 2001
From: Yuriy Skalko <yuriy.skalko at gmail.com>
Date: Wed, 4 Nov 2020 12:04:39 +0200
Subject: [PATCH 3/4] MacroData refactoring
---
src/Buffer.cpp | 2 +-
src/mathed/MacroTable.cpp | 19 +++++++------------
src/mathed/MacroTable.h | 20 ++++++++++----------
3 files changed, 18 insertions(+), 23 deletions(-)
diff --git a/src/Buffer.cpp b/src/Buffer.cpp
index b228d92392..a27e8c21b4 100644
--- a/src/Buffer.cpp
+++ b/src/Buffer.cpp
@@ -3803,7 +3803,7 @@ void Buffer::Impl::updateMacros(DocIterator & it, DocIterator & scope)
// FIXME (Abdel), I don't understand why we pass 'it' here
// instead of 'macroTemplate' defined above... is this correct?
macros[macroTemplate.name()][it] =
- Impl::ScopeMacro(scope, MacroData(const_cast<Buffer *>(owner_), it));
+ Impl::ScopeMacro(scope, MacroData(owner_, it));
break;
}
default:
diff --git a/src/mathed/MacroTable.cpp b/src/mathed/MacroTable.cpp
index fb2a9e08a4..218c8492e2 100644
--- a/src/mathed/MacroTable.cpp
+++ b/src/mathed/MacroTable.cpp
@@ -41,23 +41,18 @@ namespace lyx {
//
/////////////////////////////////////////////////////////////////////
-MacroData::MacroData(Buffer * buf)
- : buffer_(buf), queried_(true), numargs_(0), sym_(0), optionals_(0),
- lockCount_(0), redefinition_(false), type_(MacroTypeNewcommand)
+MacroData::MacroData(const Buffer * buf)
+ : buffer_(buf), queried_(true)
{}
-MacroData::MacroData(Buffer * buf, DocIterator const & pos)
- : buffer_(buf), pos_(pos), queried_(false), numargs_(0), sym_(0),
- optionals_(0), lockCount_(0), redefinition_(false),
- type_(MacroTypeNewcommand)
-{
-}
+MacroData::MacroData(Buffer const * buf, DocIterator const & pos)
+ : buffer_(buf), pos_(pos)
+{}
-MacroData::MacroData(Buffer * buf, InsetMathMacroTemplate const & macro)
- : buffer_(buf), queried_(false), numargs_(0), sym_(0), optionals_(0),
- lockCount_(0), redefinition_(false), type_(MacroTypeNewcommand)
+MacroData::MacroData(Buffer const * buf, InsetMathMacroTemplate const & macro)
+ : buffer_(buf)
{
queryData(macro);
}
diff --git a/src/mathed/MacroTable.h b/src/mathed/MacroTable.h
index b588ee2cf9..77b306fccd 100644
--- a/src/mathed/MacroTable.h
+++ b/src/mathed/MacroTable.h
@@ -38,11 +38,11 @@ enum MacroType {
class MacroData {
public:
/// Constructor to make STL containers happy
- explicit MacroData(Buffer * buf = 0);
+ explicit MacroData(Buffer const * buf = 0);
/// Create lazy MacroData which only queries the macro template when needed
- MacroData(Buffer * buf, DocIterator const & pos);
+ MacroData(Buffer const * buf, DocIterator const & pos);
/// Create non-lazy MacroData which directly queries the macro template
- MacroData(Buffer * buf, InsetMathMacroTemplate const & macro);
+ MacroData(Buffer const * buf, InsetMathMacroTemplate const & macro);
///
docstring const & definition() const { updateData(); return definition_; }
@@ -122,25 +122,25 @@ private:
/// macros.
mutable DocIterator pos_;
///
- mutable bool queried_;
+ mutable bool queried_ = false;
///
mutable docstring definition_;
///
- mutable size_t numargs_;
+ mutable size_t numargs_ = 0;
///
mutable docstring display_;
///
- latexkeys const * sym_;
+ latexkeys const * sym_ = nullptr;
///
- mutable size_t optionals_;
+ mutable size_t optionals_ = 0;
///
mutable std::vector<docstring> defaults_;
///
- mutable int lockCount_;
+ mutable int lockCount_ = 0;
///
- mutable bool redefinition_;
+ mutable bool redefinition_ = false;
///
- mutable MacroType type_;
+ mutable MacroType type_ = MacroTypeNewcommand;
};
--
2.28.0.windows.1
-------------- next part --------------
From 8f7db0891d713c280215455041662a09ac335c4f Mon Sep 17 00:00:00 2001
From: Yuriy Skalko <yuriy.skalko at gmail.com>
Date: Thu, 5 Nov 2020 12:01:21 +0200
Subject: [PATCH 4/4] Whitespace & renaming
---
src/BufferView.cpp | 6 +++---
src/insets/Inset.h | 4 ++--
src/insets/InsetText.h | 4 ++--
3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/BufferView.cpp b/src/BufferView.cpp
index 4e704e3249..bb41759870 100644
--- a/src/BufferView.cpp
+++ b/src/BufferView.cpp
@@ -504,9 +504,9 @@ void BufferView::processUpdateFlags(Update::flags flags)
// We handle this before FitCursor because the later will require
// correct metrics at cursor position.
if (!(flags & Update::ForceDraw)
- && (flags & Update::SinglePar)
- && !singleParUpdate())
- updateMetrics(flags);
+ && (flags & Update::SinglePar)
+ && !singleParUpdate())
+ updateMetrics(flags);
// Then make sure that the screen contains the cursor if needed
if (flags & Update::FitCursor) {
diff --git a/src/insets/Inset.h b/src/insets/Inset.h
index 910ca2e309..2837c2438f 100644
--- a/src/insets/Inset.h
+++ b/src/insets/Inset.h
@@ -529,8 +529,8 @@ public:
///
virtual bool allowSpellCheck() const { return false; }
- /// if this insets owns text cells (e.g. InsetText) return cell num
- virtual Text * getText(int /*num*/) const { return 0; }
+ /// if this insets owns text cells (e.g. InsetText) return cell idx
+ virtual Text * getText(int /*idx*/) const { return 0; }
/** Adds a LaTeX snippet to the Preview Loader for transformation
* into a bitmap image. Does not start the loading process.
diff --git a/src/insets/InsetText.h b/src/insets/InsetText.h
index a3daede3c5..93610ee961 100644
--- a/src/insets/InsetText.h
+++ b/src/insets/InsetText.h
@@ -114,8 +114,8 @@ public:
///
void setFrameColor(ColorCode);
///
- Text * getText(int i) const override {
- return (i == 0) ? const_cast<Text*>(&text_) : 0;
+ Text * getText(int idx) const override {
+ return (idx == 0) ? const_cast<Text*>(&text_) : nullptr;
}
///
bool getStatus(Cursor & cur, FuncRequest const & cmd, FuncStatus &) const override;
--
2.28.0.windows.1
More information about the lyx-devel
mailing list