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 !
Created attachment 104966 [details] photo of the problem =)
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.
He i want to work on this bug. Its the first time I am bug fixing on libre office :).
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
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[] = {
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 !
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
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 !
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.
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?
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 =)
Created attachment 105559 [details] callgrind output callgrind output after replacing string.copy/remove with one stringbuilder.
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?
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 !
Hey, patch should be in gerrit. https://gerrit.libreoffice.org/#/c/11334/ Have a nice day.
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.
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.
Thanks for the patch ! =)
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.
Migrating Whiteboard tags to Keywords: (EasyHack DifficultyBeginner SkillCpp TopicCleanup) [NinjaEdit]