> On April 6, 2013, 5:28 p.m., Sebastian Sauer wrote: > > libs/odf/KoXmlStreamReader.cpp, line 229 > > <http://git.reviewboard.kde.org/r/109887/diff/2/?file=131690#file131690line229> > > > > hmmm. This is a rather hot code isn't it? And it introduces > > string-concat, lookups, inserts, ... > > > > Now I am curios: is the plan to make that the default streamReader and > > if could we please measure this? > > > > Just something very simple like a > > QTime t; t.start(); > > KWDocument::openUrl(url); > > qDebug()<<t.elapsed(); > > and then compaer some documenty before vs after the patch?
Comparing with QXmlStreamReader I have really strong doubts about memory and speed of the implementation. Now a fundamental problem that follows is "I will do that when the code that uses this code is ready." what makes it rather hard to compare. I think for such an essential change in such a hot path to proper measure that idea is very much needed. The base to verify if the approach is good at all imho. "it is a very rare document where it is *not* the normal prefix" Yes, sounds like trying to optimize for the "normal case" would make indeed sense but does it optimize anything at all? If its 0:0 - no lose, no win- then indeed the code-quality argument may become an argument but imho not before. For that this code paths are to hot. - Sebastian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/109887/#review30574 ----------------------------------------------------------- On April 6, 2013, 3:56 p.m., Inge Wallin wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/109887/ > ----------------------------------------------------------- > > (Updated April 6, 2013, 3:56 p.m.) > > > Review request for Calligra, Jarosław Staniek and Jos van den Oever. > > > Description > ------- > > This patch contains a new XML stream reader based on the QXmlStreamReader > that is better suited for ODF. > > Much ODF parsing code in Calligra looks like: > > if (el.namespaceUri() == KoXml::fo && el.name == "border") { ... } > > The reason for this complicated construction is that the prefix (the "fo" in > "fo:border") is not unique but is declared at the beginning of each Xml file. > Even though "fo" is the normal prefix there is no guarantee that it is the > same in every document. > > However, it is a very rare document where it is *not* the normal prefix, so > what we want to do is to be able to write code like this: > > If (el.qualifiedName() == "fo:border") { ... } > > and make the XML stream reader or dom tree rewrite the qualified name in the > very rare cases that the prefix does not match what we want. > > This is exactly what the KoXmlStreamReader does. It allows you to write > easier and faster code while still be correct in the few cases where the > prefixes are not the expected ones. It does this by letting the user enter > the expected namespace declarations and then compare those to the actual > namespace declarations in the document. If they match everything will be as > fast as possible. If they don't, it will be slower but still correct. > > As an extra feature I have allowed the user to declare some extra namespaces > (see fixNamespace() in KoXmlReader.cpp). This will let documents created by > old versions of OpenOffice.org be read even though they use other namespaces. > > I have code that uses this file but that is not yet ready for review. I > wanted to put this up early to get feedback while the rest of the yet > unfinished code is maturing. > > > Diffs > ----- > > libs/odf/CMakeLists.txt 3680486 > libs/odf/KoXmlStreamReader.h PRE-CREATION > libs/odf/KoXmlStreamReader.cpp PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/109887/diff/ > > > Testing > ------- > > Not much. I will do that when the code that uses this code is ready. This > review is for getting feedback on the ideas and implementation details. > > > Thanks, > > Inge Wallin > >
_______________________________________________ calligra-devel mailing list calligra-devel@kde.org https://mail.kde.org/mailman/listinfo/calligra-devel
