<div dir="rtl"><div dir="ltr">Sean, thanks for reminding this, Alp did commit a class derived from raw_svector_ostream conatining an internal SmallString he called <span style="color:rgb(0,0,0);white-space:pre-wrap">small_string_ostream. The commit was reverted after a day due to a disagreement about the commit approval and apparently abandoned.</span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><font color="#000000"><span style="white-space:pre-wrap"><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140623/223393.html">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140623/223393.html</a></span></font><br></div><div dir="ltr"><font color="#000000"><span style="white-space:pre-wrap"><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140623/223557.html">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140623/223557.html</a></span><br></font></div><div dir="ltr"><font color="#000000"><span style="white-space:pre-wrap"><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140630/223986.html">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140630/223986.html</a></span><br></font></div><div dir="ltr"><font color="#000000"><span style="white-space:pre-wrap"><br></span></font></div><div dir="ltr"><font color="#000000"><span style="white-space:pre-wrap">The class Alp comitted did solve the possible mismatch between the SmallString and the stream by making the SmallString private to the class. This however did not treat the root problem, the duplication of the information about the buffer between SmallString and the stream.</span></font></div><div dir="ltr"><font color="#000000"><span style="white-space:pre-wrap"><br></span></font></div><div dir="ltr"><font color="#000000"><span style="white-space:pre-wrap">I can make performance measurements but even if the performance difference is neglible (and looking at all the code paths and conditionals of non-inlined functions at raw_ostream.cpp that's hard to believe), </span></font><font color="#000000"><span style="white-space:pre-wrap">the current </span></font><span style="color:rgb(0,0,0);white-space:pre-wrap">SmallString-</span><font color="#000000"><span style="white-space:pre-wrap">raw_svector_ostream</span></font><span style="color:rgb(0,0,0);white-space:pre-wrap"> is simply wrong.</span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">Personally, after the previous "Alp" discussion I found and fixed several such issues in my out-of-tree code only to make new similar new error earlier this year, which I caught only months after, when Rafael committed the pwrite which reminded me the issue. </span><span style="color:rgb(0,0,0);white-space:pre-wrap">Due ot the information duplication Rafael </span><span style="color:rgb(0,0,0);white-space:pre-wrap">commit also had a bug and Mehdi reports similar experience. B</span><span style="color:rgb(0,0,0);white-space:pre-wrap">ack then </span><span style="color:rgb(0,0,0);white-space:pre-wrap">Alp reported similar problems he found in LLVM code which were hopefully fixed otherwise.</span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">In addition to the information duplication, the design is quite confusing to use</span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">- Should one use the stream.str() function or the SmallString itself? </span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">- flush or str?</span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">- How do you properly clear the combination for reuse (that was my mistake, I forgot to resync after OS.clear())?</span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">It's safe to say this design pattern is very easy to get wrong one way or another, we got burned by it multiple times and it should be replaced.</span><br></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">Alp suggested a derived class containing its own SmallString. That's a simple and effective approach to avoid the bugs mentioned but does not fix the memory or runtime issues. Small as they may be, why have them at a fundemental data structure?</span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">I was thinking about going further avoiding all duplication with a templated class derived with its own internal buffer storage.</span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">Rafael suggested a more modular approach, a derived adpater class adapter to a *simple* buffer (or nullptr for fully-dynamic operation) which also won't duplicate any information </span><span style="color:rgb(0,0,0);white-space:pre-wrap">the buffer is dumb and has no state. </span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap"><br></span></div><div dir="ltr"><span style="color:rgb(0,0,0);white-space:pre-wrap">That solution sounds simple, efficient and safe to use. The implementation would be actually simpler then </span><font color="#000000"><span style="white-space:pre-wrap">raw_svector_ostream</span></font><span style="color:rgb(0,0,0);white-space:pre-wrap"> since all the coordination logic is not required anymore.</span></div><div dir="ltr"><br></div></div><div class="gmail_extra"><div dir="ltr"><br><div class="gmail_quote">2015-04-20 22:17 GMT+03:00 Sean Silva <span dir="ltr"><<a href="mailto:chisophugis@gmail.com" target="_blank">chisophugis@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 .8ex;border-left:1px #ccc solid;border-right:1px #ccc solid;padding-left:1ex;padding-right:1ex"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Sun, Apr 19, 2015 at 7:40 AM, Yaron Keren <span dir="ltr"><<a href="mailto:yaron.keren@gmail.com" target="_blank">yaron.keren@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="rtl"><div dir="ltr">A very common code pattern in LLVM is</div><div dir="ltr"><br></div><div dir="ltr"><font face="monospace, monospace"> SmallString<128> S;</font></div><div dir="ltr"><font face="monospace, monospace"> raw_svector_ostream OS(S);<br></font></div><div dir="ltr"><font face="monospace, monospace"> OS<< ...</font></div><div dir="ltr"><font face="monospace, monospace"> Use OS.str()</font></div><div dir="ltr"><br></div><div dir="ltr">While raw_svector_ostream is smart to share the text buffer itself, it's inefficient keeping two sets of pointers to the same buffer:</div><div dir="ltr"><br></div><div dir="ltr"> In SmallString: void *BeginX, *EndX, *CapacityX<br></div><div dir="ltr"> In raw_ostream: char *OutBufStart, *OutBufEnd, *OutBufCur<br></div><div dir="ltr"><br></div><div dir="ltr">Moreover, at runtime the two sets of pointers need to be coordinated between the SmallString and raw_svector_ostream using raw_svector_ostream::init, raw_svector_ostream::pwrite, raw_svector_ostream::resync and raw_svector_ostream::write_impl.</div><div dir="ltr">All these functions have non-inlined implementations in raw_ostream.cpp. </div><div dir="ltr"><br></div><div dir="ltr">Finally, this may cause subtle bugs if S is modified without calling OS::resync(). This is too easy to do by mistake.</div><div dir="ltr"><br></div><div dir="ltr">In this frequent case usage the client does not really care about S being a SmallString with its many useful string helper function. It's just boilerplate code for raw_svector_ostream. But it does cost three extra pointers, some runtime performance and possible bugs.</div></div></blockquote><div><br></div></span><div>I agree the bugs are real (Alp proposed something a while back regarding this?), but you will need to provide measurements to justify the cost in runtime performance. One technique I have used in the past to measure these sorts of things I call "stuffing": take the operation that you want to measure, then essentially change the logic so that you pay the cost 2 times, 3 times, etc. You can then look at the trend in performance as N varies and extrapolate back to the case where N = 0 (i.e. you don't pay the cost).</div><div><br></div><div>For example, in one situation where I used this method it was to measure the cost of stat'ing files (sys::fs::status) across a holistic build, using only "time" on the command line (it was on Windows and I didn't have any tools like DTrace available that can directly measure this). In order to do this, I changed sys::fs::status to call stat N times instead of 1, and measured with N=1 N=2 N=3 etc. The result was that the difference between the N and N+1 versions was about 1-2% across N=1..10 (or whatever I measured). In order to negate caching and other confounding effects, it is important to try different distributions of stats; e.g. the extra stats are on the same file as the "real" stat vs. the extra stats are on nonexistent files in the same directory as the "real" file vs. parent directories of the "real" file; if these match up fairly well (they did), then you have some indication that the "stuffing" is measuring what you want to measure.</div><div><br></div><div>So e.g. if you think the cost of 3 extra pointers is significant, then "stuff" the struct with 3, 6, 9, ... extra pointers and measure the difference in performance (e.g. measure the time building a real project).</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-- Sean Silva</div></font></span><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="rtl"><div dir="ltr"><br></div><div dir="ltr">To solve all three issues, would it make sense to have raw_ostream-derived container with a its own SmallString like templated-size built-in buffer?</div><div dir="ltr"><br></div></div>
</blockquote></span></div><br></div></blockquote></div></div></div>