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

[Stephan Bergmann]

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.

-Stephan