Reading files create useless copies

[franz1981]

Version

4.x and 5.x

Context

vert.x/vertx-core/src/main/java/io/vertx/core/file/impl/FileSystemImpl.java

Line 879 in 8ce1eb1

return Buffer.buffer(bytes);

shows that we perform a double copy of the byte[] since Buffer perform another copy of it

Steps to reproduce

No response

Do you have a reproducer?

No response

[doxlik]

Hello dear @franz1981 and Vert.x community, I really love this project and want to help it as much as possible. I don't have super deep expertise in all Java / Netty file optimizations (while I actively dive into it) so I just ask ChatGPT for possible variants to solve this copy issue and one of its suggestions seems for me quite good. I know anyone of us can just go and ask ChatGPT but I reviewed its code and for me it seems as quite strong choice and I am interested in your opinion.

// in io.vertx.core.file.impl.FileSystemImpl

import io.netty.buffer.ByteBuf;
import io.netty.buffer.Unpooled;
import io.vertx.core.buffer.Buffer;
import io.vertx.core.buffer.impl.BufferImpl;

import java.io.IOException;
import java.nio.MappedByteBuffer;
import java.nio.channels.FileChannel;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardOpenOption;

private BlockingAction<Buffer> readFileInternal(String path) {
  Objects.requireNonNull(path);
  return new BlockingAction<Buffer>() {
    @Override
    public Buffer perform() {
      final Path target;
      try {
        target = resolveFile(path).toPath();
      } catch (Throwable t) {
        throw new FileSystemException(getFileAccessErrorMessage("read", path), t);
      }

      try {
        final long size = Files.size(target);

        // If the file fits into a single ByteBuffer (<= 2 GiB), prefer mmap to avoid heap allocation + copy.
        if (size <= Integer.MAX_VALUE) {
          try (FileChannel ch = FileChannel.open(target, StandardOpenOption.READ)) {
            // Map the whole file read-only. This yields a direct MappedByteBuffer; no heap array allocation.
            MappedByteBuffer mbb = ch.map(FileChannel.MapMode.READ_ONLY, 0, size);
            // Wrap without copying into a Netty ByteBuf, then into a Vert.x Buffer.
            ByteBuf bb = Unpooled.wrappedBuffer(mbb);    // no copy
            return new BufferImpl(bb);                   // no copy
          }
        }

        // Fallback for very large files (>2 GiB) or if mmap is not desired:
        // Read into a byte[] once, but still avoid the second copy by WRAPPING the array.
        byte[] bytes = Files.readAllBytes(target);           // one copy from kernel to heap
        ByteBuf bb = Unpooled.wrappedBuffer(bytes);          // no additional copy
        return new BufferImpl(bb);

      } catch (IOException e) {
        throw new FileSystemException(getFileAccessErrorMessage("read", path), e);
      }
    }
  };
}

[franz1981]

Thanks @doxlik for it, I have sadly worked quite a lot with memory mapping in my life (I am an ex-developer of Active MQ Artemis were I was the maintainer of the mmap journal...) and it hides manysubtle problems:

• mmap unmapping is not deterministic (unless using memory segments in newer APIs)
• it usually pollute the OS page cache of lot of mapped pages just for reading it
• the increased virtual memory usage can make DevOps go nuts (at best)
• memory mapping accesses appear as normal read of bytes and this can cause severe "time to safe point" pauses, since the JVM is not prepared to know that will happen, delaying GC and othe important activities
• memory mapping in one thread, reading and maybe unmapping in other threads is prone to a problem called TLB shootdown which can cause OS level pauses

In addition, the semantic of the returned buffer is different:

• before: reading the buffer and modifying it was still possible (I believe?)
• now (w mmap): if you modify the buffer it will throw an exception

which is a rather different type of behaviour

You can try submitting these concerns to ChatGTP and see if it agrees of fund some not clear and I can help clarify each of them

In order to fix it I would suggest instead a small incremental improvement
i.e. // one copy from kernel to heap code `

It already reduce by far the number of copies (to one) and still leave the caller to rely on the same semantic.

[doxlik]

Thank you very much for your explanation and sharing experience, I will as you said do a research for myself about mmap regarding concerns that you mentioned within ChatGPT and if something will be unclear I will ask you (thank you for this suggestion).

And regarding incremental small fix - totally agree, I think for now this approach of just wrapping buffers instead of copying already can give significant benefit without using fragile and dangerous stuff as mmap.

[franz1981]

And regarding incremental small fix - totally agree, I think for now this approach of just wrapping buffers instead of copying already can give significant benefit without using fragile and dangerous stuff as mmap.

Yep, so, would you like opening a PR? Both me and @vietj love contributions ❤️

[doxlik]

Oh, it is quite exciting suggestion, just despite I had read lot of Vert.x source code and being its big fan, this will be my first contribution to any open source and especially such big one, so I may have some simple questions.

1. Can I just create branch from master or some other branch or I need to do it via fork?
2. Just if needed I can sign any of Eclipse contribution agreement (I saw this was signed by guy who was working on Postgres COPY PR)
3. Need to say honestly, I ready to write any tests and match any requirement, just because of my job I may not be able to let's say join some calls and etc currently. I know it is just a couple lines of code, just again it is my first contribution, so want it to not be dissapointment to anyone.

[franz1981]

no worries @doxlik you did right :)