Guillaume Munch wrote:
> Le 16/10/2015 21:52, Georg Baum a écrit :
>> Guillaume Munch wrote:
>>
>>> I did not realise that there were supposed to be so many InsetMaths live
>>> at the same time. Although the increased memory use is quite small for
>>> modern standard, I agree that it is a bad idea to increase it needlessly
>>> both in principle and in the long run.
>>>
>>> In fact I only need uids on InsetMathNest, which should represent a much
>>> smaller population.
>>
>> There can still be lots of nest insets.
>
> Hard figures: on the document that used to create 2'000'000 uids on
> opening, and which only creates about 470'000 now (after patch #6 and
> moving uids to InsetMathNest), there are in fact 48'700 live uids
> (≈390kB). This is for a math-heavy document with hundreds of macros and
> whose size in terms of combined lyx files is 530kB.
>
> I miss hard figures about the desired memory consumption.
Sorry, I don't have the time right now to really measure, but I think it is
not needed, since the numbers you got are OK IMHO.
>> IMHO knowing the formula which
>> produced the error is already a big improvement over the current
>> situation.
>
> We already had that, since the hull was already identified with its
> position in the paragraph.
Ah OK, in that case it does indeed not make sense to have it in the hull.
> Grids are not the only producers of line breaks although they represent
> a main target. But at various places, the inset does not record its own
> id, but the id of its enclosing inset, so I potentially call id() for
> any kind of InsetMathNest.
>
> Then I think that it makes sense to add it to InsetMathNest, because any
> other location would produce diminished results (contrary to the move
> InsetMath→InsetMathNest which effectively removes useless uids).
OK, that is convincing.
> But if the memory usage is still too high, various solutions:
> * pointer comparison (what is your opinion on pointer comparison vs.
> using a counter?)
> * variable-length integers encoding.
> * pointer comparison, but somehow keep the possibility to use a counter
> instead for using jointly with the other debug options.
The two last ones are probably too complicated. As I see it, we do now have
a trade off between easier debugging (ids), and less memory consumption
(pointers). Apart from that, both solutions would work. Since you are the
one who is probably doing most of the debugging work please decide, I am now
fine with either solution.
>> The underlying problem is that a temporary InsetMathString is generated
>> at all, for the sole purpose to use InsetMathString::write(). A proper
>> fix would be to factor out InsetMathString::write() into a helper
>> function, that can be used both from a vector of InsetMathChar and
>> InsetMathString::write(). Then one would not need any inset copying at
>> all for this purpose.
>
> My analysis diverges. See above. The real point is that we must iterate
> in-place over the original MathData to avoid a deep copy.
Here we agree.
> If we had a
> helper function writeString() then we would still have to write an
> iteration in the style of my patch. It would be the same code up to a
> minor rephrasing. Whether we call InsetMathString::write() or a function
> writeString() is secondary.
With the last sentence we disagree. Having no temp InsetMathString is a key
point for understanding the code IMHO. If you have no temp inset, then
there is suddenly no need to circumvent anything imposed by the MathAtom and
MathData design anymore. If you have a temp inset, it is not obvious that
the only reason for having it is to use the write() method.
Since I believe that we somehow misunderstood each other I attached a sketch
of what I meant. In order to keep the patch small and to show the main
intent, I did keep writeString() in InsetMathString.cpp. If this gets
applied then it should of course move to MathExtern.cpp.
The reason why I think that this is more readable is that there is no temp
inset involved, and there is only one simple loop instead of two nested
ones. There are also no comments needed stating that validity of the
iterator. With this patch, the only "complicated" part is in write() in
MathExtern.cpp, but it is exactly at the right place: If you look at it, you
can see easily why collecting MathCharInsets is needed.
Regarding the other parts of the patches I have some nitpicks: Please no
semicolons which are not needed, so
uid_type id() const { return uid_.id(); }
instead of
uid_type id() const { return uid_.id(); };
Also, an operator==() which does not compare anything is rather dangerous
IMHO, I think it is safer to simply exclude the id in the comparison
operator of the owner.
Then, is id() needed to be virtual in InsetMath? When using it you need to
distinguish anyway between a valid and an invalid one, so can this maybe
simply be kept nonvirtual in InsetMathNest?
I do not know the TeXRow stuff too well, so I won't comment on that, but I
believe you should apply 0003-Splitting-otexstream-into-otexrowstream-and-
otexstre.patch immediately.
Georg
diff --git a/src/mathed/InsetMathString.cpp b/src/mathed/InsetMathString.cpp
index 53aba58..1323236 100644
--- a/src/mathed/InsetMathString.cpp
+++ b/src/mathed/InsetMathString.cpp
@@ -100,13 +100,19 @@ void InsetMathString::mathmlize(MathStream &) const
void InsetMathString::write(WriteStream & os) const
{
+ writeString(str_, os);
+}
+
+
+void writeString(docstring const & s, WriteStream & os)
+{
if (!os.latex() || os.lockedMode()) {
- os << (os.asciiOnly() ? escape(str_) : str_);
+ os << (os.asciiOnly() ? escape(s) : s);
return;
}
- docstring::const_iterator cit = str_.begin();
- docstring::const_iterator end = str_.end();
+ docstring::const_iterator cit = s.begin();
+ docstring::const_iterator end = s.end();
// We may already be inside an \ensuremath command.
bool in_forced_mode = os.pendingBrace();
diff --git a/src/mathed/InsetMathString.h b/src/mathed/InsetMathString.h
index 9f94a74..7eff709 100644
--- a/src/mathed/InsetMathString.h
+++ b/src/mathed/InsetMathString.h
@@ -59,6 +59,10 @@ private:
docstring str_;
};
+
+/// Write \p s (which may contain math or text contents in LaTeX syntax) to \p os
+void writeString(docstring const & s, WriteStream & os);
+
} // namespace lyx
#endif
diff --git a/src/mathed/MathExtern.cpp b/src/mathed/MathExtern.cpp
index 1904843..44880a4 100644
--- a/src/mathed/MathExtern.cpp
+++ b/src/mathed/MathExtern.cpp
@@ -1382,11 +1382,24 @@ namespace {
void write(MathData const & dat, WriteStream & wi)
{
- MathData ar = dat;
- extractStrings(ar);
wi.firstitem() = true;
- for (MathData::const_iterator it = ar.begin(); it != ar.end(); ++it) {
- (*it)->write(wi);
+ docstring s;
+ for (MathData::const_iterator it = dat.begin(); it != dat.end(); ++it) {
+ InsetMathChar const * const c = (*it)->asCharInset();
+ if (c)
+ s += c->getChar();
+ else {
+ if (!s.empty()) {
+ writeString(s, wi);
+ s.clear();
+ wi.firstitem() = false;
+ }
+ (*it)->write(wi);
+ wi.firstitem() = false;
+ }
+ }
+ if (!s.empty()) {
+ writeString(s, wi);
wi.firstitem() = false;
}
}
