Bug 80907 - avoid unnecessary allocations in OpenXML import filter
Summary: avoid unnecessary allocations in OpenXML import filter
Status: NEW
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
unspecified
Hardware: Other All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2014-07-04 10:14 UTC by Michael Meeks
Modified: 2018-08-07 00:30 UTC (History)
5 users (show)

See Also:
Crash report or crash signature:


Attachments
kcachegrind picture (459.35 KB, image/png)
2014-07-04 10:27 UTC, Michael Meeks
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2014-07-04 10:14:46 UTC
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.

writerfilter/source/ooxml/OOXMLPropertySetImpl.hxx
   + has code for the OOXMLPropertyImpl etc.

git log -u -1 c4b7910c120be1c5a8462c20b12734139632a4de

shows a change from Kohei doing something similar inside calc.

Thanks !
Comment 1 Michael Meeks 2014-07-04 10:27:31 UTC
Created attachment 102262 [details]
kcachegrind picture
Comment 2 Commit Notification 2014-07-16 15:02:11 UTC
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.
Comment 3 David Tardon 2014-07-22 10:46:20 UTC
(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.
Comment 4 Michael Meeks 2014-07-22 14:14:06 UTC
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.
Comment 5 David Tardon 2014-07-23 15:23:13 UTC
(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.
Comment 6 Michael Meeks 2014-07-23 21:17:57 UTC
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.
Comment 7 Jeffrey Stedfast 2014-07-26 21:57:42 UTC
I've fixed bug #80908 which was related to this (updated OOXMLBooleanValue and OOXMLIntegerValue to have ::Create() methods that return a static Pointer_t).
Comment 8 David Tardon 2014-07-28 08:27:31 UTC
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.
Comment 9 Björn Michaelsen 2014-12-02 10:53:08 UTC
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
Comment 10 Robinson Tryon (qubit) 2015-12-14 04:59:02 UTC Comment hidden (obsolete)
Comment 11 Robinson Tryon (qubit) 2016-02-18 14:51:34 UTC Comment hidden (obsolete)