[llvm] r212816 - raw_svector_ostream: grow and reserve atomically

[Alexander Kornienko]

On Fri, Jul 11, 2014 at 6:49 PM, Alp Toker <alp at nuanti.com> wrote:

>
> On 11/07/2014 19:41, Alexander Kornienko wrote:
>
>  On Fri, Jul 11, 2014 at 6:35 PM, Alp Toker <alp at nuanti.com <mailto:
>> alp at nuanti.com>> wrote:
>>
>>
>>     On 11/07/2014 19:26, Alexander Kornienko wrote:
>>
>>         I wonder whether I missed the review thread for this patch or
>>         this kind of patch is generally accepted to be committed
>>         without any review?
>>
>>
>>     I'm OK with this change, but any review is welcome. This is a
>>     supporting commit towards fixing the really old FIXMEs related to
>>     scratch buffer overallocation.
>>
>>
>> Can you explain why changing growth from exponential to linear is fine
>> here? For large sizes it will lead to O(size) write time.
>>
>
> There's no change to linear growth. See the implementation of reserve()
> which calls into SmallVector's grow():
>
>   /// grow - Grow the allocated memory (without initializing new
>   /// elements), doubling the size of the allocated memory.
>   /// Guarantees space for at least one more element, or MinSize more
>   /// elements if specified.
>   void grow(size_t MinSize = 0);
>
>
Thank you for the explanation. This probably deserves a short note in the
patch description, as it may be not immediately obvious.


>
>
>>
>>         On Fri, Jul 11, 2014 at 4:02 PM, Alp Toker <alp at nuanti.com
>>         <mailto:alp at nuanti.com> <mailto:alp at nuanti.com
>>
>>         <mailto:alp at nuanti.com>>> wrote:
>>
>>             Author: alp
>>             Date: Fri Jul 11 09:02:04 2014
>>             New Revision: 212816
>>
>>             URL: http://llvm.org/viewvc/llvm-project?rev=212816&view=rev
>>             Log:
>>             raw_svector_ostream: grow and reserve atomically
>>
>>             Including the scratch buffer size in the initial reservation
>>             eliminates the
>>             subsequent malloc+move operation and offers a healthier
>>         constant
>>             growth with
>>             less memory wastage.
>>
>>             When doing this, take care to avoid invalidating the
>>         source buffer.
>>
>>             Modified:
>>                 llvm/trunk/lib/Support/raw_ostream.cpp
>>
>>             Modified: llvm/trunk/lib/Support/raw_ostream.cpp
>>             URL:
>>         http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
>> Support/raw_ostream.cpp?rev=212816&r1=212815&r2=212816&view=diff
>>                    ==============================
>> ================================================
>>             --- llvm/trunk/lib/Support/raw_ostream.cpp (original)
>>             +++ llvm/trunk/lib/Support/raw_ostream.cpp Fri Jul 11
>>         09:02:04 2014
>>             @@ -729,24 +729,26 @@ void raw_svector_ostream::resync() {
>>              }
>>
>>              void raw_svector_ostream::write_impl(const char *Ptr,
>>         size_t Size) {
>>             -  // If we're writing bytes from the end of the buffer
>>         into the
>>             smallvector, we
>>             -  // don't need to copy the bytes, just commit the bytes
>>         because
>>             they are
>>             -  // already in the right place.
>>             -  if (Ptr == OS.end()) {
>>             -    assert(OS.size() + Size <= OS.capacity() && "Invalid
>>             write_impl() call!");
>>             -    OS.set_size(OS.size() + Size);
>>             +  size_t NewSize = OS.size() + Size;
>>             +  size_t NewReservation = NewSize + 64;
>>             +
>>             +  bool NoOverlap = Ptr + Size < OS.begin() || Ptr >
>>         OS.begin() +
>>             OS.capacity();
>>             +
>>             +  if (NoOverlap) {
>>             +    assert(!GetNumBytesInBuffer());
>>             +    OS.reserve(NewReservation);
>>             +    memcpy(OS.end(), Ptr, Size);
>>             +    OS.set_size(NewSize);
>>             +  } else if (Ptr == OS.end()) {
>>             +    // Grow the buffer to include the scratch area
>>         without copying.
>>             +    assert(NewSize <= OS.capacity() && "Invalid
>>         write_impl() call!");
>>             +    OS.set_size(NewSize);
>>             +    OS.reserve(NewReservation);
>>                } else {
>>             -    assert(GetNumBytesInBuffer() == 0 &&
>>             -           "Should be writing from buffer if some bytes
>>         in it");
>>             -    // Otherwise, do copy the bytes.
>>             -    OS.append(Ptr, Ptr+Size);
>>             +    OS.append(Ptr, Ptr + Size);
>>             +    OS.reserve(NewReservation);
>>                }
>>
>>             -  // Grow the vector if necessary.
>>             -  if (OS.capacity() - OS.size() < 64)
>>             -    OS.reserve(OS.capacity() * 2);
>>             -
>>             -  // Update the buffer position.
>>                SetBuffer(OS.end(), OS.capacity() - OS.size());
>>              }
>>
>>
>>
>>             _______________________________________________
>>             llvm-commits mailing list
>>         llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>         <mailto:llvm-commits at cs.uiuc.edu
>>
>>         <mailto:llvm-commits at cs.uiuc.edu>>
>>         http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>>
>>     --     http://www.nuanti.com
>>     the browser experts
>>
>>
>>
> --
> http://www.nuanti.com
> the browser experts
>
>


-- 
Alexander Kornienko | Software Engineer | alexfh at google.com | Google
Germany, Munich
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140712/3094bde1/attachment.html>