Guillaume Munch wrote: > Le 14/10/2015 21:35, Georg Baum a écrit : >> >> Please measure memory consumption. I am a bit afraid that this increases >> memory usage a lot, since math insets contain very tiny bits of >> information, and you may need _lots_ of them even for simple formula. > > 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. Why not InsetMathHull? The accuracy of the error locations is limited at LaTeX side anyway, so it does not make sense to be too accurate at LyX side. IMHO knowing the formula which produced the error is already a big improvement over the current situation. If you insist on having something more fine grained than a hull inset, then please use InsetMathGrid. These can still occur a number of times in a document, but not as often as nest insets, and grids are the ones that produce most newlines. Also, they do already have expensive members, so another 8 bytes should not be too problematic. >> Why do you need a new member variable at all? Why can't you just use the >> memory address? This would not consume memory and still be a unique id. > > Yes, it would make sense to use pointers IMO. There was two reasons for > the UniqueId class: I needed to experiment with different copy semantics > before I finally decided for the current behaviour; and also I find that > using the address looked kludgy compared to the current code (e.g. the > paragraph id). Another aspect is that during debugging, having an id > that starts from 0 is easier to read and keep track of. I suggest seeing > how it goes with having ids only for InsetMathNest. Does that make sense? Except that I would use a different inset (see above) yes. >> The new code looks rather complicated to me. I think the old one should >> be kept. > > ExtractStrings forces write to repeat a deep copy on a recursive call. I > would advise against keeping write as it is. There already was an > unexplained time explosion when generating previews in a document with a > lot of math macros, before Enrico rationalised the macro definition > output during preview. What was not explained is that it took more than > a time linear in the size of the generated preview tex file, in the > tests that I did at the time. Not to say that it's necessarily related > (though it may be), but this can be a concrete issue. > > I am skeptical about "complicated" given that it is the same as before, > except that it was doing a deep copy before (which I call "complicated" > in this context). As for readability, see the attached diff where it is > rephrased using a separate function. It is still not as readable as the old version IMO. >> What I do not understand is why this code is executed at all for LaTeX >> output. I believe that it can even produce wrong PDF contents. > > Do you have an example of how it might invalidate the latex output? Now I had more time to look at the code, and have to admit that my memory was partly wrong. While all the other extractSomething methods are indeed for CAS etc, extractStrings() is different: It does not produce invalid LaTeX, but it is indeed needed to produce correct LaTeX for consecutive InsetMathChar. So it is correct that it is being used. >> Therefore, if >> LaTeX output is fixed to not call the free function write() at all, then >> extractStrings() can be kept and will not interfere. > > Sorry, that's beyond my current knowledge of LyX internals. I am > confident in my patches because I make small local changes. I suggest > keeping my fix for now, made more readable as attached, unless somebody > is able to provide a patch along your lines in a timely manner. 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. Your fix makes the code less understandable than it currently is. I fear that this will make a proper future fix less likely. If you are currently not confident enough doing the real fix (although you got a really good understanding of the math stuff in no time), who will be confident enough in the future? I would either keep the current version, or go for the full fix and refactor InsetMathString::write(). Georg
