Introduce StringBuilder utility around raw_string_ostream

[Alp Toker]

On 12/06/2014 18:25, Chandler Carruth wrote:
> On Thu, Jun 12, 2014 at 4:16 PM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>     Okay, but that's still causing std::string heap allocations.
>
>
> Ok, all of the discussion thus far has focused exclusively on 
> preventing bugs, so that was where I focused my suggestion.
>
>
>     With my approach I can do:
>
>       typedef SmallStringBuilder<128> StringBuilder;
>
>
> You can also achieve the same results with my suggestion... Nothing I 
> suggested is *really* tied to the std::string-composing variant of the 
> raw_*_ostream helpers...
>
>
>     That turns a whole lot of allocations and strlens across the board
>     into no-ops with a simple centralised change (patch attached,
>     based on Yaron Keren's suggestion). This really *does* make an
>     impact and there are secondary benefits to consider like reduced
>     fragmentation (though I don't have figures, it's clearly going to
>     help). Stack allocation always wins for small strings outside of
>     recursion scenarios.
>
>
> You don't have to teach me this. ;] I've spent a decent time turning 
> heap allocations into stack allocations throughout Clang and LLVM.
>
>     And the patch also enforces clean storage reset() which is a
>     really tricky operation to do manually with an ostream (requires
>     sequential flush, storage reset, and sync -- do we really expect
>     people to hand-write that without bugs?)
>
>
> This is certainly a handy operation. Why not add it to the existing 
> stream classes? It seems useful regardless of whether you want 
> internal or external storage.

The existing stream classes are really light append-only non-seekable 
streams. reset() is a higher-level feature that makes onerous 
assumptions about storage, both "what it can do" and "what we're allowed 
to do with it".

So to put reset() or seeking operations inside the primitive ostream 
subclasses would be quite a layering violation.

>
>     And it'll be visible to further PGO optimisations (anything that
>     tunes a SmallString will apply here too).
>
>
> I don't know what this means... I don't think that my suggestion is in 
> any way different from the perspective of PGO....
>
>
>     On the other hand the approach you suggested just adds some
>     syntactic sugar, at the cost of tying the code closer to
>     std::string heap storage -- exactly what I'm incrementally working
>     to avoid.
>
>
> Again, I'm moderately sure you can achieve all the non-std::string 
> storage you want with my suggestion. What I'm suggesting is a general 
> technique for embedding storage inside of a stream object in a way 
> that can easily be used with the existing APIs. It should be 
> generalizable to whatever stream objects are useful to embed storage 
> within....

I can't see how that'd be done without making a muddle of the ostream 
primitive classes. Adding optional storage to a "simple adaptor class" 
strikes me as a poor idea because it'd no longer be usable be a thin 
interface that's easily inheritable.

At 25 LoC the proposed implementation is still pretty tight even after 
adding the stack allocation facility. And it doesn't hack up ostream 
classes, and fixes various points of use, all clear pluses to me.

Alp.



-- 
http://www.nuanti.com
the browser experts