Created attachment 102262 [details]
kcachegrind picture

Fahad Al-Saidi committed a patch related to this issue.
It has been pushed to "master":

http://cgit.freedesktop.org/libreoffice/core/commit/?id=c04a0895df86f40faef1bc093f7010f374df9d0b

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:
http://wiki.documentfoundation.org/Testing_Daily_Builds
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
> object/allocation.

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)
{
    if (val)
    {
        static OOXMLValue::Pointer_t aTrue(new OOXMLBooleanValue(val));
        return aTrue;
    }
    else
    {
        static OOXMLValue::Pointer_t aFalse(new OOXMLBooleanValue(val));
        return aFalse;
    }
}

(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

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 =)