Issue 34006 - suggest converting INetURLObject to use rtl::OUString
Summary: suggest converting INetURLObject to use rtl::OUString
Status: CLOSED FIXED
Alias: None
Product: utilities
Classification: Unclassified
Component: code (show other issues)
Version: 680m58
Hardware: All All
: P3 Trivial (vote)
Target Milestone: OOo 2.0
Assignee: thorsten.martens
QA Contact: Unknown
URL:
Keywords:
Depends on: 33774
Blocks:
  Show dependency tree
 
Reported: 2004-09-09 12:26 UTC by caolanm
Modified: 2004-12-03 19:08 UTC (History)
2 users (show)

See Also:
Issue Type: PATCH
Latest Confirmation in: ---
Developer Difficulty: ---


Attachments
the substantial part of the suggestion (162.23 KB, patch)
2004-09-09 12:28 UTC, caolanm
no flags Details | Diff
the trivial parts of it (35.93 KB, application/x-compressed)
2004-09-09 12:29 UTC, caolanm
no flags Details

Note You need to log in before you can comment on or make changes to this issue.
Description caolanm 2004-09-09 12:26:55 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
Comment 1 caolanm 2004-09-09 12:28:23 UTC
Created attachment 17616 [details]
the substantial part of the suggestion
Comment 2 caolanm 2004-09-09 12:29:41 UTC
Created attachment 17617 [details]
the trivial parts of it
Comment 3 caolanm 2004-09-09 12:35:38 UTC
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/)
Comment 4 matthias.huetsch 2004-09-09 16:42:49 UTC
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
Comment 5 matthias.huetsch 2004-09-09 16:43:22 UTC
Adding myself to CC...
Comment 6 Stephan Bergmann 2004-09-13 15:51:24 UTC
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.
Comment 7 caolanm 2004-09-30 09:17:42 UTC
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.
Comment 8 caolanm 2004-09-30 09:29:39 UTC
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)
Comment 9 Stephan Bergmann 2004-09-30 09:42:21 UTC
Sounds fine.  Thanks again for taking the time to do this kind of drudging
improvement work :)
Comment 10 caolanm 2004-10-01 15:37:30 UTC
Complete in ineturl1 (based on m54), will update to m55 to get latest urlobj.?xx
changes etc...
Comment 11 caolanm 2004-10-14 12:39:05 UTC
reopen to reassign to QA
Comment 12 caolanm 2004-10-14 12:39:41 UTC
Reassigned to qa of ineturl1
Comment 13 thorsten.martens 2004-10-27 10:11:03 UTC
Fix can´t be verified due to missing working cws !
Comment 14 caolanm 2004-10-27 11:00:08 UTC
will resync to m58
Comment 15 caolanm 2004-11-01 14:10:34 UTC
new linux installset available at
http://ooomisc.services.openoffice.org/pub/OpenOffice.org/cws/upload/gtkfpicker2/
Comment 16 quetschke 2004-11-12 14:58:47 UTC
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' 
Comment 17 caolanm 2004-11-12 15:09:13 UTC
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)
Comment 18 thorsten.martens 2004-11-23 11:44:29 UTC
Checked and verified in cws ineturl1.
Comment 19 thorsten.martens 2004-12-03 10:17:26 UTC
Verified in 680m64 -> OK !
Comment 20 quetschke 2004-12-03 19:08:37 UTC
Just out of curiosity.

> Verified in 680m64 -> OK !

How, EIS says the CWS is not yet integrated!