Apache OpenOffice (AOO) Bugzilla – Issue 34006
suggest converting INetURLObject to use rtl::OUString
Last modified: 2004-12-03 19:08:37 UTC
With experiments using callgrind, converting INetURLObject to use rtl::OUString and rtl::OUStringBuffer reduces the number of instructions used from startup to hide call on splashscreen from 1,375,826,046 instructions to 1,238,756,746. The following patch implements a change like that in tools, and the following zip is a set of trivial patches to compile the rest of OOo cleanly with the change
Created attachment 17616 [details] the substantial part of the suggestion
Created attachment 17617 [details] the trivial parts of it
cmc->mhu: Any interest in an approach like this to improving performance ? (callgrind is a backend for kcachegrind, all available from http://kcachegrind.sf.net/)
Hi Caolan, Yes, I am interested, of course. The patches look good, though I haven't read through all of it at the same level of detail. Thus, I would leave the final review to Stephan (sb) as the code owner. The 'trivial' part looks indeed trivial, and doesn't need further review. Just one comment of mine: Could you replace the (few?) remaining occurences of 'ByteString' with 'rtl:OString / rtl::OStringBuffer'? That would make the interface perfectly consistent (no more old tools strings). I guess, the main improvement comes from using StringBuffers for the most expensive parts of string manipulation. And a difference of ~10% in number of instructions is indeed significant, and shows we should probably do similar changes at other places as well. So, unless Stephan has any veto, I'd ask you to go ahead and incorporate your patches via the usual cws mechanisms. Thanks, Matthias
Adding myself to CC...
sb->cmc: Generally, using OStringBuffer within INetUrlObject is a good idea. I have a very old Hamburg-internal P5 performance issue about this, but never found time to actually do anything. I'm not sure it would have been necessary to completely switch INetURLObject from tools to rtl strings, but since you already did all the work... I'm also not sure whether it pays off to replace the remaining ByteString with rtl::OString (if you do, you should be able to drop the include of tools/string from urlobj.hxx, which might require addition of #include "tools/string.hxx" at some places downstream). Some details about inetspeedups.tools.patch: 1 Why setHost(rtl::OUStringBuffer const &...) instead of setHost(rtl::OUString const &...)? 2 Why an additional parseHost(..., rtl::OUStringBuffer & rCanonic)? (Also, its body has an unnecessary rtl::OUString ctor in "rCanonic = rtl::OUStringBuffer(rtl::OUString(sTemp));".) 3 Isn't lcl_Erase(rtl::OUStringBuffer &rBuf, sal_Int32 index) the same as rBuf.setLength(index)? 4 I am confused that the modification of the signature of INetURLObject::parsePath (last argument, from UniString* to rtl::OUStringBuffer&) is apparently missing from urlobj.hxx. Also, the use of a pointer here was intentional, to give a clue at the places where the function is called that the respective parameter is an out-parameter (via the additional "&"). The only available tests for INetURLObject are in tools/workben/urltest.cxx. Please make sure that they still work.
taking your points under advisement, and agreeing that a change from ByteString to OString is more trouble than its worth (as you say String to OUString in the interface was more trouble than it was worth, but now its done so no extra effort to do) I'll go ahead and setup a workspace for this and commit in the changes with changes to rectify the points unless there's any reason not to.
1. indeed 2. yep 3. yes 4. is an artifact of the patch, the header post-patch matches the code forgot of course that I had added some const to the rtl::OUStringBuffer get methods in my local copy when examing this (issue 33774)
Sounds fine. Thanks again for taking the time to do this kind of drudging improvement work :)
Complete in ineturl1 (based on m54), will update to m55 to get latest urlobj.?xx changes etc...
reopen to reassign to QA
Reassigned to qa of ineturl1
Fix can´t be verified due to missing working cws !
will resync to m58
new linux installset available at http://ooomisc.services.openoffice.org/pub/OpenOffice.org/cws/upload/gtkfpicker2/
Hi Caolan, the windows build of cws_src680_ineturl1 breaks with: Making: ../../wntmsci10.pro/slo/inettbc.obj guw.pl /cygdrive/c/PROGRA~1/MICROS~1.NET/Vc7/bin/cl.exe -Zm500 -wd4251 -wd4275 -wd4290 -wd4786 -wd4800 -Zc:forScope -GR -c -nologo -Gs -Gy -I. -I. -I../inc -I../../inc -I../../WIN/inc -I../../wntmsci10.pro/inc -I. -I/cygdrive/e/w1/SRC680_m58/solver/680/wntmsci10.pro/inc/stl -I/cygdrive/e/w1/SRC680_m58/solver/680/wntmsci10.pro/inc/external -I/cygdrive/e/w1/SRC680_m58/solver/680/wntmsci10.pro/inc -I/cygdrive/e/w1/SRC680_m58/solenv/wntmsci10/inc -I/cygdrive/e/w1/SRC680_m58/solenv/inc -I/cygdrive/e/w1/SRC680_m58/res -I/cygdrive/e/w1/SRC680_m58/solver/680/wntmsci10.pro/inc/stl -I/cygdrive/c/j2sdk1.4.2_05/include/win32 -I/cygdrive/c/j2sdk1.4.2_05/include -I/cygdrive/c/PROGRA~1/MICROS~3/include -I/cygdrive/c/PROGRA~1/MICROS~1.NET/Vc7/include -I/cygdrive/c/PROGRA~1/MICROS~1.0SD/include -I. -I../../res -I. -Ob1 -Zi -Fd../../wntmsci10.pro/misc/_ooo_st_filepicker.PDB -Oxs -Oy- -Gd -GX -DWNT -DWNT -DNT351 -DMSC -DM1310 -DINTEL -D_USE_NAMESPACE -D_X86_=1 -DFULL_DESK -DSTLPORT_VERSION=400 -DWINVER=0x400 -D_WIN32_IE=0x400 -D_MT -DCPPU_ENV=msci -DSUPD=680 -DPRODUCT -DNDEBUG -DPRODUCT_FULL -DOSL_DEBUG_LEVEL=0 -DOPTIMIZE -DEXCEPTIONS_ON -DCUI -DSOLAR_JAVA -DSRC680 -DUPD=\"680\" -DMINOR=\"m58\" -DBUILD_ID=\"8821\" -DSHAREDLIB -D_DLL_ -DWIN32 -D_MT -D_DLL -DWIN32 -D_MT -D_DLL -DMULTITHREAD -W3 -Fo../../wntmsci10.pro/slo/inettbc.obj /cygdrive/e/w1/SRC680_m58/svtools/source/filepicker/inettbc.cxx guw.pl /cygdrive/c/PROGRA~1/MICROS~1.NET/Vc7/bin/cl.exe @/tmp/mkb01884 inettbc.cxx e:\w1\SRC680_m58\svtools\source\filepicker\inettbc.cxx(658) : error C2593: 'operator +=' is ambiguous e:\w1\SRC680_m58\solver\680\wntmsci10.pro\inc\tools\string.hxx(565): could be 'String &String::operator +=(const sal_Unicode *)' e:\w1\SRC680_m58\solver\680\wntmsci10.pro\inc\tools\string.hxx(563): or 'String &String::operator +=(const String &)' while trying to match the argument list '(String, rtl::OUString)' e:\w1\SRC680_m58\svtools\source\filepicker\inettbc.cxx(829) : warning C4244: 'argument' : conversion from 'sal_Int32' to 'USHORT', possible loss of data dmake: Error code 2, while making '../../wntmsci10.pro/slo/inettbc.obj'
Fixed: svtools/source/filepicker/inettbc.cxx -r1.33.68.2 commited. Code hiding inside if WNT (vq: you can open new bugs against me for any other problems that show up)
Checked and verified in cws ineturl1.
Verified in 680m64 -> OK !
Just out of curiosity. > Verified in 680m64 -> OK ! How, EIS says the CWS is not yet integrated!