From koffice-devel Wed Apr 28 08:35:04 2010 From: "Thomas Zander" Date: Wed, 28 Apr 2010 08:35:04 +0000 To: koffice-devel Subject: Re: Review Request: load table:table as part of a draw:frame in the Message-Id: <20100428083504.13488.27737 () localhost> X-MARC-Message: https://marc.info/?l=koffice-devel&m=127244367818169 > On 2010-04-25 09:44:54, Thomas Zander wrote: > > trunk/koffice/libs/flake/KoShapeFactoryBase.cpp, line 96 > > > > > > Now we no longer return a odfNameSpace() as a QString I'd like to suggest that the namespace is kept as a const char *. > > The advantage there is that we can use the pointers from KoXmlNS and thus only do pointer comparison for namespaces. Which means that we avoid converting to utf16 (QString native) on call to setOdfElementNames and avoid a string compare. > > That should make a significant impact in startup time too :) > > > > This, together with the previous suggestion, would make the elements stored like; > > QHash > > Thorsten Zachmann wrote: > There was always a QString used for the namespace > > -QString KoShapeFactoryBase::odfNameSpace() const > > so this patch doesn't change anything in this regard. > > It is needed as a QString in the registry as there we have a QString we need to compare it too. So it makes sense to do the conversion only once. > > Thomas Zander wrote: > I completely agree it was always a QString, didn't mean to imply anything else or that you broke something. > I'm just looking at this from the point of view that with the new data structure you introduced the thing may need an extra look as it does look rather imposing with nested templates now. > In the registry we do too many implicit conversions which really makes things slower than they need be. So, yes, some extra code is needed in the registry (specifically KoShapeRegistry::Private::createShapeInternal) to make this use the char*, but it does certainly look like its worth it. > To be clear; if you see a > e.namespaceURI() == KoXmlNS::draw > the right parameter is a char*, so this causes a conversion of the char* to QString (utf16) and then a string compare. This is done quite often, so thats my point. It would be much cheaper to replace all this with simple pointer compares. > > Anyway, I hope you see my point and think it makes sense to make things even better. I won't insist on anything, I just wanted to give you an idea for improvement. > > Thorsten Zachmann wrote: > Unfortunately the left parameter in > e.namespaceURI() == KoXmlNS::draw > is a QString so the KoXmlNS::draw needs to be converted to a QString to compare the string. Even if we would also have a char * for the namespaceURI we would need to compare the content and not only the pointers. > > I done some benchmarking with > > QString == char * and QString == QString and they is no difference in speed and we get over 10000000 comparisons in a second. > > > > Well, yes, there is no difference if you compare QStrings one way or the other. Naturally :) You have to convert the e.namespaceURI() to one of the char* pointers from the KoXmlNS *first* before you see any advantage :) After that any comparison will avoid the conversion and thus be dramatically faster. In one method we convert the same char* 3 times, so there certainly is more speed to be gained on loading. Anyway, this is not really relevant for your fix, I'll put it on my TODO list as future research. - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/3805/#review5215 ----------------------------------------------------------- On 2010-04-25 06:55:45, Thorsten Zachmann wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviewboard.kde.org/r/3805/ > ----------------------------------------------------------- > > (Updated 2010-04-25 06:55:45) > > > Review request for KOffice. > > > Summary > ------- > > Make it possible to load a table:table inside a draw:frame. To make this possible it is needed to extend KoShapeFactory and KoShapeRegistry that multiple namespaces -> tag's can be supported by one shape. > > > This addresses bug 231680. > https://bugs.kde.org/show_bug.cgi?id=231680 > > > Diffs > ----- > > trunk/koffice/libs/flake/KoShapeFactoryBase.h 1118236 > trunk/koffice/libs/flake/KoShapeFactoryBase.cpp 1118236 > trunk/koffice/libs/flake/KoShapeRegistry.cpp 1118236 > trunk/koffice/libs/flake/tests/TestKoShapeFactory.cpp 1118236 > trunk/koffice/libs/kotext/opendocument/KoTextLoader.cpp 1118236 > trunk/koffice/plugins/textshape/TextShape.h 1118236 > trunk/koffice/plugins/textshape/TextShape.cpp 1118236 > trunk/koffice/plugins/textshape/TextShapeFactory.cpp 1118236 > > Diff: http://reviewboard.kde.org/r/3805/diff > > > Testing > ------- > > > Thanks, > > Thorsten > > _______________________________________________ koffice-devel mailing list koffice-devel@kde.org https://mail.kde.org/mailman/listinfo/koffice-devel