The writerfilter imports DOCX files and it spends quite lot of un-necessary cycles allocating un-necessary things and then freeing them very shortly afterwards =)
The use of boost::shared_ptr creates a chunk of waste here - it requires allocating an extra object to hold a ref-count; and we can easily do this just as well with an intrusive pointer that is part of the same object/allocation.
All this sort of stuff:
OOXMLValue::Pointer_t xValue(new OOXMLBooleanValue(pValue));
Should be switched from a boost::shared_ptr to an intrusive ptr - also since this is all inside one thread domain we should inline non-atomic inc/dec. operations there too.
writerfilter/source/ooxml/OOXMLFactory.cxx (attributes) shows this stuff.
+ has code for the OOXMLPropertyImpl etc.
git log -u -1 c4b7910c120be1c5a8462c20b12734139632a4de
shows a change from Kohei doing something similar inside calc.
Created attachment 102262 [details]
Fahad Al-Saidi committed a patch related to this issue.
It has been pushed to "master":
fdo#80907 Implemented OOXMLFactory using boost::intrusive_ptr.
The patch should be included in the daily builds available at
http://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
Affected users are encouraged to test the fix and report feedback.
(In reply to comment #0)
> The writerfilter imports DOCX files and it spends quite lot of un-necessary
> cycles allocating un-necessary things and then freeing them very shortly
> afterwards =)
> The use of boost::shared_ptr creates a chunk of waste here - it requires
> allocating an extra object to hold a ref-count; and we can easily do this
> just as well with an intrusive pointer that is part of the same
It does not, if the shared_ptr is created by boost::make_shared<Foo>(...) instead of boost::shared_ptr(new Foo(...)). Then there is only one allocation.
Fair cop - but for very high frequency things like the OOXML attributes / values etc. [ the ones tackled so far are ~never used sadly ;-] - I want intrusive pointers for 2x reasons:
a) to avoid allocations and
b) to avoid atomic interlock operations on ref counting
We should never be allocating / ref / unref'ing one of two boolean values eg. ;-)
Otherwise make_shared seems like a great idea.
(In reply to comment #4)
> Fair cop - but for very high frequency things like the OOXML attributes /
> values etc. [ the ones tackled so far are ~never used sadly ;-] - I want
> intrusive pointers for 2x reasons:
> a) to avoid allocations and
> b) to avoid atomic interlock operations on ref counting
> We should never be allocating / ref / unref'ing one of two boolean values
> eg. ;-)
So I would say that tweaking the smart ptr. impl. goes in a wrong way. What is needed is to avoid direct use of ctors and new/make_shared and instead use creator functions than can do some extra work to avoid needless allocations. E.g., someting like the following would ensure there are never more than 2 allocations of objects of OOXMLBooleanValue:
OOXMLValue::Pointer_t OOXMLBooleanValue::create(bool val)
static OOXMLValue::Pointer_t aTrue(new OOXMLBooleanValue(val));
static OOXMLValue::Pointer_t aFalse(new OOXMLBooleanValue(val));
(Of course, for this to really work, clone() would have to be changed to return Pointer_t as well. But I do not think that naked pointers are used anywhere at all, so this is not a problem.)
OOXMLIntegerValue and the few others that wrap intergers could use some sort of cache.
Sure - I just don't like 31337 interlocked atomic referencing on something that is not thread safe, its pure waste =) either way - we can wait until we see that in the profile again I guess. The fruit is so low-hanging here we can win whatever we do.
I've fixed bug #80908 which was related to this (updated OOXMLBooleanValue and OOXMLIntegerValue to have ::Create() methods that return a static Pointer_t).
Eh, there had been a specific easy hack for that? I did not notice; sorry I hijacked this one then (but the description was rather generic)... Let's move this back to a simple "avoid double allocation on creating new values" then.
adding LibreOffice developer list as CC to unresolved Writer EasyHacks for better visibility.
see e.g. http://nabble.documentfoundation.org/minutes-of-ESC-call-td4076214.html for details
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup )
JanI is default CC for Easy Hacks (Add Jan; remove LibreOffice Dev List from CC)
I think this is done. OOXMLValue derives from SvRefBase which has an intrusive reference count and is not multi- threaded. Just like Michael Meeks wanted.
Thanks Deb! its been open too long, and wonderful to remember Jeff's contribution in passing =)