Summary: | avoid unnecessary allocations in OpenXML import filter | ||
---|---|---|---|
Product: | LibreOffice | Reporter: | Michael Meeks <michael.meeks> |
Component: | Writer | Assignee: | Not Assigned <libreoffice-bugs> |
Status: | NEW --- | QA Contact: | |
Severity: | normal | ||
Priority: | medium | CC: | dtardon, Fahad.alsaidi, libreoffice |
Version: | unspecified | ||
Hardware: | Other | ||
OS: | All | ||
See Also: | https://bugs.freedesktop.org/show_bug.cgi?id=80908 | ||
Whiteboard: | EasyHack DifficultyBeginner SkillCpp TopicCleanup target:4.4.0 | ||
i915 platform: | i915 features: | ||
Attachments: | kcachegrind picture |
Description
Michael Meeks
2014-07-04 10:14:46 UTC
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 |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.