[PATCH] D91693: [Support] Add reserve() method to the raw_ostream.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added a comment.

> It's not immediately clear to me which data you're concerned about loading into memory, but it's worth remembering that objcopy will need to have all the data (or at least its size) in its internal representation before it can write most bits, at least for ELF objcopy, because the section header offset field is part of the ELF header, but the section header table itself is at the end.

I mean bits of the resulting object. f.e. output ELF file data.

My understanding is that this(having all the data in internal representation) might be not 
necessary for other formats, f.e. MachO/Wasm. Following, is the existing code for MachO :

  Expected<std::unique_ptr<MemoryBuffer>>
  object::writeUniversalBinaryToBuffer(ArrayRef<Slice> Slices) {
    SmallVector<char, 0> Buffer;
    raw_svector_ostream Out(Buffer);
  
    if (Error E = writeUniversalBinaryToStream(Slices, Out))  <<< data is written to the memory stream
      return std::move(E);
      
    return std::make_unique<SmallVectorMemoryBuffer>(std::move(Buffer));
  }    
  ......    
    Expected<std::unique_ptr<MemoryBuffer>> B =
        writeUniversalBinaryToBuffer(Slices);
    if (!B)
      return B.takeError();
    if (Error E = Out.allocate((*B)->getBufferSize()))
      return E;
    memcpy(Out.getBufferStart(), (*B)->getBufferStart(), (*B)->getBufferSize());  
    ^^^^^^^^^
    data is copied into the output buffer  

Using stream as a destination would allow us not to use the intermediate memory object.
i.e. If it is not necessary to update the file header after the end of the file is written/or if that size 
could be precalculated in the start - then we do not need to load all file data into the 
internal buffers.

For the ELF case, there would be three alternatives(in streams solution)(*) :

1. If the section header offset could somehow be precalculated then we are lucky and

do not need to update it after the file is written.

2. We could store the file data into the memory buffer, update the field and then copy this buffer

into the destination stream.

3. Use raw_pwrite_stream as the destination. That would allow us to update the section header

offset after the file is written.

So we would still be able to successfully handle the ELF case and we do not require loading whole 
file bits into the memory for other cases.

> This can be solved by overloading:
>
> // In all examples, types of first two parameters are arbitrarily chosen, the third parameter, when present, is the important one).
>
> // Execute objcopy then write to file Out.
> Error executeObjcopyOnBinary(const CopyConfig &Config, const Binary &In, StringRef Out) {
>
>   // Do some objcopy stuff to finalise the objcopy layout.
>   Object Obj = executeObjcopy(Config, In);
>   size_t Size = Obj.getOutputSize();
>   raw_mmap_ostream OS(Out, Size);
>   writeOutput(OS, Obj);
>
> }
>
> // Execute objcopy then write to raw_ostream Out. raw_mmap_ostream won't be usable here, since the output size isn't known up front.
> Error executeObjcopyOnBinary(const CopyConfig &Config, const Binary &In, raw_ostream &Out) {
>
>   Object Obj = executeObjcopy(Config, In);
>   writeOutput(Out, Obj);
>
> }
>
> // If a user wanted to write the output to an existing raw_mmap_ostream, they'd need to call the lower-level API directly:
> void aFunction(...) {
>
>   ...
>   Object Obj = executeObjcopy(Config, In);
>   size_t Size = Obj.getOutputSize();
>   ...
>   raw_mmap_ostream Out(FileName, Size + ...);
>   ...
>   writeOutput(Out, Obj);
>
> }
>
> It's worth noting that we cannot have Expected<raw_ostream &> executeObjcopyOnBinary(const CopyConfig &Config, const Binary &In); where the function itself creates the stream and returns it, because something needs to own the backing storage.

In this design we have a new entity "Object" in the interface:

  Object Obj = executeObjcopy(Config, In);

This new "Object" would force additional copying. F.e. following usage would become useless:

  void aFunction(...) {
    ...
    Object Obj = executeObjcopy(Config, In);
    size_t Size = Obj.getOutputSize();
    ...
    raw_mmap_ostream Out(FileName, Size + ...);
    ...
    writeOutput(Out, Obj); <<< data should be copied from Object Obj into the raw_mmap_ostream Out;
  }

The advantage of using a memory-mapped file is that we could write directly to the memory owned by the memory-mapped file
and this memory would be stored by the system. Using an intermediate Object, would require us to do extra copying from that Object into a memory-mapped file. In the streams scenario we do not need to do extra copying:

  executeObjcopyOnBinary(CopyConfig &Config, object::Binary &In, raw_ostream &Out) {
    Writer->finalize(Out); <<< calls Out.reserve(XXX);
  
    Writer->write(Out);  <<< writes directly to the memory mapped file data
  }
  
  void aFunction(...) {
    ...
    raw_mmap_ostream Out(FileName);
    executeObjcopy(Config, In, Out);
  }



> I don't think it's possible to start writing to a stream before the size of the data is known (see above). Does that mean streams are still justified?

My understanding is yes, streams are still justified. see (*).

> I don't think resizeable memory-mapped files are useful in this context, because the size needs to be known before writing anyway. The exception is if we think calling the objcopy guts is too complicated for users that want to write to an existing memory mapped file, but I think it is possible to keep it simple, like in my example above.

But we already know the size inside objcopy before writing. 
Currenty we call Buf.allocate(totalSize()) before starting writing.
With streams solution, we would call reserve(totalSize()) in that place.

> This is merely a nice-to-have, and not itself directly related to the issue at hand. It can be implemented later. People who want the efficiency bonus of reserving could use the same approach as I did in the above example with the adding to an existing memory mapped file, by calling the lower-level API directly. They will usually have access to their underlying backing storage anyway (e.g. the std::vector) and so can just reserve it directly.

yes, that could be implemented later. 
But that would be required to have the same performance results for streams solution and the current implementation.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D91693/new/

https://reviews.llvm.org/D91693