Download it now!
Bug 82854 - speed up OutputDevice::GetDefaultFont
Summary: speed up OutputDevice::GetDefaultFont
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
4.3.0.2 rc
Hardware: Other All
: medium normal
Assignee: Michael Jaumann
URL: https://www.google.de/?gfe_rd=cr&ei=7...
Whiteboard: target:4.4.0
Keywords: difficultyBeginner, easyHack, skillCpp, topicCleanup
Depends on:
Blocks:
 
Reported: 2014-08-20 09:16 UTC by Michael Meeks
Modified: 2015-12-15 23:57 UTC (History)
1 user (show)

See Also:
Crash report or crash signature:


Attachments
photo of the problem =) (479.78 KB, image/png)
2014-08-20 09:18 UTC, Michael Meeks
Details
callgrind output (390.89 KB, image/png)
2014-09-01 13:02 UTC, Michael Jaumann
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Meeks 2014-08-20 09:16:53 UTC
The callgrind profile in comment 20 of bug#79761 (load it in kcachegrind) shows an unexpectedly high cost for:

vcl/source/outdev/font.cxx (OutputDevice::GetDefaultFont)

850m pcycles for 18k calls - 47m pcycles for each call - seems a lot for (what looks like) a fairly innoculous method ;-)

Possibly we should be caching the result on the OutputDevice - I guess adding some printfs might help there (?).

Quite probably we should be constructing strings more sensibly in GetEnglishSearchFontName - 550m cycles or so [ 132k calls ]. Quite possibly we could do the search without turning into an English search name with more clevers (?).

Anyhow - worth someone having a poke to dig through that as an easy hack I suspect. Caolan ( if you can restrain yourself from just fixing it ;-) - I'd appreciate your advice as an easy hack mentor ;-)

Thanks !
Comment 1 Michael Meeks 2014-08-20 09:18:12 UTC
Created attachment 104966 [details]
photo of the problem =)
Comment 2 Michael Meeks 2014-08-21 22:11:32 UTC
GetEnglishSearchName is gratuitously inefficient ;-)

It loves to copy the string left/right cf.

unotools/source/misc/fontdefs.cxx

It also badly needs some unit tests in:

unotools/qa/unit/test.cxx

or something - that would need creating, worth coping the vcl/qa/cppunit/canvasbitmaptester.cxx or somesuch there [ and associated gnumake pieces ].

I imagine that using OUStringBuffer's "remove" methods instead of always allocating and freeing the string with 'copy' would make a pleasing difference.
Comment 3 Michael Jaumann 2014-08-28 11:23:10 UTC
He i want to work on this bug. Its the first time I am bug fixing on libre office :).
Comment 4 Michael Jaumann 2014-08-28 13:24:36 UTC
I just edited the unotools/source/misc/fontdefs.cxxI and using an OUStringBuffer instead of the OUStringBuffer OUString.

To vaildate it, i wanted to write a unit test. So where can i find informations/examples how to write an unit test, and what would be an valid/invalid combination (Parameters/Returnvalues) for the GetEnglishSearchFontName() function.

Thanks
Comment 5 Michael Meeks 2014-08-28 13:39:26 UTC
Writing unit tests is awesome - thanks for that =)

I'd create a new directory in unotools/qa/ and copy an existing unit test + CppUnitTest_foo.mk file from somewhere else.

In terms of shoving junk through it - I would read the code, and try to exercise each code path; whack a ton of strings through it and just have a:

static struct { const char *before; const char *after } aValidate[] = {
Comment 6 Michael Meeks 2014-08-28 13:41:05 UTC
No doubt having edited the code you'll be able to work out what to pack that array with =) add some fprintfs; and (of course) ensure the unit test works before & after.

It looked (to me) as if there were quite some wins possible there. From a performance perspective - if you run your list of 100x strings a few thousand times, and callgrind it - you should be able to see the hotspots pretty easily =)

Thanks !
Comment 7 Michael Jaumann 2014-08-29 11:26:52 UTC
s/qa/ and copy an existing unit test + CppUnitTest_foo.mk file from somewhere else.

In terms of shoving junk through it - I would read the code, as/qa/ and copy an existing unit test + CppUnitTest_foo.mk file from somewhere else.
s/qa/ and copy an existing unit test + CppUnitTest_foo.mk file from somewhere else.

In terms of shoving junk through it - I would read the code, a
In terms of shoving junk through it - I would read the code, a
Comment 8 Michael Jaumann 2014-08-29 12:29:04 UTC
im sry, that last commend i wrote, was just a missclick.

Now i think i understand unit tests and i created one. Created/copied a makefile.

If i want to make , i am running into an error:   undefined reference to `GetEnglishSearchFontName(rtl::OUString const&)'

I included <unotools/fontdefs.hxx> in my test. So could it be a problem of my makefile?. I didnt  changed this part of the .mk file. 

$(eval $(call gb_CppunitTest_use_libraries,unotools_fontdefs, \ ...



Would be nice if u could help me a bit. 


Thanks !
Comment 9 Michael Meeks 2014-08-29 15:40:08 UTC
Hey - Michael =)

> If i want to make , i am running into an error:   undefined reference to
> `GetEnglishSearchFontName(rtl::OUString const&)'

Clearly we're not linking with the object that defines that. If you do:

objdump -T <foo.so>

you can see what symbols a shared library exports - rather a useful trick ;-)

In this case it is in the 'utl' library (cf. the /fontdefs in Library_utl.mk) - so hopefully adding 'utl' to the used libraries list will fix that =)

Looking forward to reading the patch.
Comment 10 Michael Jaumann 2014-09-01 09:29:20 UTC
Hey,

thanks that all worked for me.

I reproduced the steps in bug#79761 and created a callgrind log.

Unfortunate the callgrind.out.<pid>  is empty.

Any suggestions?
Comment 11 Michael Meeks 2014-09-01 09:59:42 UTC
Interesting; did callgrind complete cleanly ? until callgrind exits nothing is written to the file - it is just created zero length. You have to be pretty patient, this thing runs 150x slower or so.

In general, I'd be inclined to profile your unit test with callgrind, not LibreOffice itself. Instead, I'd use fprintf to dump the pattern of calls to that method, and the return values so you can write a good performance test that matches that to profile =) and also to see if there are any really awfully embarrassing patterns in there we can exploit =)
Comment 12 Michael Jaumann 2014-09-01 13:02:28 UTC
Created attachment 105559 [details]
callgrind output

callgrind output after replacing string.copy/remove with one stringbuilder.
Comment 13 Michael Jaumann 2014-09-01 14:25:19 UTC
i tried another time and now callgrind+steps from bug#7976 run through. 
-> the kcachegrind image is stored in the attachements.(The outputfile from callgrind was to huge).

For me it looks like a cost reduction. What to do next?
Comment 14 Michael Meeks 2014-09-08 10:55:13 UTC
The profile looks great =) OTOH - where is the patch ? in general, developers talk code - and its not possible to review / merge a callgrind profile =)
So the next step should be to push that to gerrit - looking forward to reading it =) Thanks !
Comment 15 Michael Jaumann 2014-09-08 11:15:23 UTC
Hey, 
patch should be in gerrit.
https://gerrit.libreoffice.org/#/c/11334/
Have a nice day.
Comment 16 Commit Notification 2014-09-08 13:24:34 UTC
Michael Jaumann committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82854 - use OUStringBuffer in fontdefs.cxx plus unit-test.



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 17 Commit Notification 2014-09-08 13:24:50 UTC
Michael Meeks committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82854 - cleanup & review bits.



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 18 Michael Meeks 2014-09-08 14:06:16 UTC
Thanks for the patch ! =)
Comment 19 Commit Notification 2014-09-09 15:54:33 UTC
Michael Jaumann committed a patch related to this issue.
It has been pushed to "master":

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

fdo#82854 extended unit-tests



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 20 Robinson Tryon (qubit) 2015-12-15 23:57:28 UTC
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup)
[NinjaEdit]