[Libreoffice] inconsistency between rtl::OString/rtl::OUString and optimization opportunities ?

Stephan Bergmann stephan.bergmann.secondary at googlemail.com
Wed Aug 31 06:41:12 PDT 2011

On Aug 31, 2011, at 12:04 PM, Caolán McNamara wrote:
> rtl_string_new_WithLength/rtl_uString_new_WithLength create an
> rtl/String/rtl_uString of the given length, but set the full buffer
> contents to 0s[1]

No idea whether specifying that the "values of all characters are set to 0" was meant to be actually useful (and is exploited somewhere) or was misguided overeagerness.

> meanwhile I see that we have x_rtl_uString_new_WithLength in
> inc/i18nutil/x_rtl_ustring.h which is a non-zeroed out variant of
> rtl_uString_new_WithLength so there's an uninitialized buffer available
> to play with.
> We can assign ownership of the rtl_uString to an OUString with
> OUString( rtl_uString * str, __sal_NoAcquire ) and skip calling acquire,
> i.e. OUString has *two* methods to take ownership of a rtl_uString
> without calling acquire.
> the public OUString( rtl_uString * str, __sal_NoAcquire )
> the private OUString( rtl_uString * value, DO_NOT_ACQUIRE * )
> as well as the usual add-add-refcount version of
> OUString( rtl_uString * str ) 
> While OString has only the private 
> OString( rtl_String * value, DO_NOT_ACQUIRE * )
> as well as the usual add-add-refcount version of
> OString( rtl_String * str ), so the same hack isn't available.

No reason to not add a OString(…, __sal_NoAcquire) variant, too, I would say.

> I'm vaguely thinking of moving that i18nutil x_rtl stuff into
> comphelper/string or something, hard-coding its refcount argument to 0
> or 1 and fixing up its uses to consistently use one or the other public
> OUString acquire/noacquire ctors and add a OString variant so that
> either of the belows is possible to skip copying buffers around the
> place.
> #if thinko-one
> rtl_String *pStr = foo_rtl_string_new_WithLength(nLen/*, refCount=0*/);
> //call acquire, refCount of 1, OString::dtor destroys buffer
> rtl::OString aRet(pStr);
> pStr->length = rStrm.Read(pStr->buffer, nLen);
> #else
> rtl_String *pStr = foo_rtl_string_new_WithLength(nLen/*, refCount=1*/);
> //don't call acquire, refCount stays 1, OString::dtor destroys buffer
> rtl::OString aRet(pStr, SAL_NO_ACQUIRE);
> pStr->length = rStrm.Read(pStr->buffer, nLen);
> #endif

I would go with the second choice.  foo_rtl_string_new_WithLength creates and returns a new resource that the caller is responsible of eventually releasing again, be it via an explicit rtl_string_release or via the SAL_NO_ACQUIRE trick.  Also, that way its behavior is similar to plain rtl_string_new_WithLength, and adding the corresponding optimization for nLen=0 in the future would be straightforward.

> //if unlikely length != nLen make deep copy of string to release excess
> //mem, nobody seems to worry about excess for rtl::O[U]String buffers,
> //but much less opportunity to have a large chunk of unused
> //buffer there.
> C.
> [1] with a loop, I suppose its no real optimization there to use memset
> or rtl_allocateMemory for that case ?

Wouldn't hurt to use memset instead, I'd say.  Even if all compilers reduced the loop to something as efficient, using memset would still be shorter than that verbose loop.


More information about the LibreOffice mailing list