Avoiding extra byte copy inside FileSystemImpl.readFileInternal

[doxlik]

Motivation:

In this short PR I did optimization that previously was discussed with @franz1981 in this issue: #5665 regarding avoiding of extra bytes copy in FileSystemImpl.readFileInternal method.

Additional notes:

1. I could not either "mvn package" or "mvn test" as there multiple Http2 related files inside master is not compiling now.
2. It is my totally first contribution, sorry if I missed something.

Conformance:

Everything is signed, my eclipse account is identical to github one.

[franz1981]

@vietj : this is ok? Or you prefer to add a new constructor to `BufferImpl' which still uses the existing Vertx heap types eg VertxUnsafeHeapByteBuf/VertxHeapByteBuf ?
The reason I'm asking is:

• Unpooled::wrappedBuffer still have Netty reference count checks under the hood, implemented - which have a cost
• IIRC Vertx itself contains checks to make sure the allocator of a vertx buffer is "known" to vertx e.g.

  vert.x/vertx-core/src/main/java/io/vertx/core/net/impl/VertxHandler.java

  Lines 48 to 67 in ff78924

  	public static ByteBuf safeBuffer(ByteBuf byteBuf) {
  	Class<?> allocClass;
  	if (byteBuf != Unpooled.EMPTY_BUFFER &&
  	((allocClass = byteBuf.alloc().getClass()) == AdaptiveByteBufAllocator.class
  	|| allocClass == PooledByteBufAllocator.class
  	|| byteBuf instanceof CompositeByteBuf)) {
  	try {
  	if (byteBuf.isReadable()) {
  	ByteBuf buffer = VertxByteBufAllocator.DEFAULT.heapBuffer(byteBuf.readableBytes());
  	buffer.writeBytes(byteBuf, byteBuf.readerIndex(), byteBuf.readableBytes());
  	return buffer;
  	} else {
  	return Unpooled.EMPTY_BUFFER;
  	}
  	} finally {
  	byteBuf.release();
  	}
  	}
  	return byteBuf;
  	}

One pain point is that AFAIK Netty's heap buffers (one of the two variant) doesn't expose in public to directly set the wrapped buffer - and that's why Netty itself is using only non-Unsafe variants in Unpooled::wrappedBuffer(byte[]): if we want to do this right in Vertx we could leverage a single implementation (i.e.non-unsafe) as well or modify Netty to expose it for Unsafe variants (I can do it if needed)

[doxlik]

@franz1981 thank you for your comments, I removed unrelated changes. Actually they were made by Intelij IDEA code autoformatting and I thought that maybe they are okay, but I agree that they create unnecessary noise, so next time I will try to avoid any of them.

[franz1981]

Fyi @vietj

[tsegismont]

Thanks @doxlik and @franz1981

How about changing the perform method body to:

      public Buffer perform() {
        try {
          Path target = resolveFile(path).toPath();
          try (FileChannel fc = FileChannel.open(target, StandardOpenOption.READ)) {
            BufferInternal res = BufferInternal.buffer(); // create with file size hint?
            long length = fc.size();
            res.getByteBuf().writeBytes(fc, 0, (int) length); // check actual size before the cast
            return res;
          }
        } catch (IOException e) {
          throw new FileSystemException(getFileAccessErrorMessage("read", path), e);
        }
      }

It's ok to use BufferInternal in Vert.x impl classes and Netty can take care of loading and writing the file bytes appropriately.

[franz1981]

@tsegismont

It looks like

https://github.com/netty/netty/blob/f922892f44743411ce89e8259d705ed9c9d3763e/buffer/src/main/java/io/netty/buffer/UnpooledHeapByteBuf.java#L292

that's going to allocate the internal NIO buffer which usually is not used in the Vertx heap ones (the internal NIO buffer is something used by Netty while sending data over the wire while communicating w NIO JDK layers...).
But It's still a good suggestion and still an improvement vs what we got - while retaining the same behaviour as we have now

@doxlik wdyt?

[doxlik]

@franz1981 Regarding the suggestion, what if file really big and bigger than 2 gb? Then here " res.getByteBuf().writeBytes(fc, 0, (int) length); // check actual size before the cast" even if we check the size, we still need to fallback to something that can handle it, either wrapping or have some "while" loop until fully read the file to the buffer.

Regarding generally low level ByteBuffs stuff, it is super interesting but I have almost no solid knowledge of all its internals, so in terms of buffers / allocators optimization I fully trust to you.

[tsegismont]

@franz1981 Regarding the suggestion, what if file really big and bigger than 2 gb? Then here " res.getByteBuf().writeBytes(fc, 0, (int) length); // check actual size before the cast" even if we check the size, we still need to fallback to something that can handle it, either wrapping or have some "while" loop until fully read the file to the buffer.

I think it's safe to stick to the behavior of java.nio.file.Files#readAllBytes

If the size is bigger than java.nio.file.Files#MAX_BUFFER_SIZE, an OutOfMemoryError error is thrown

[doxlik]

@tsegismont then if we want stick to behavior of java.nio.file.Files#readAllBytes, then should we move to the channel approach as you suggested or we can simply leave the current code that already use readAllBytes?

[doxlik]

@franz1981 @vietj I also wanted to ask, what will be further process of the review / PR? I mean is it kinda in direct queue of pending PR's for Julien's review / approval or we are waiting for Vert.x 5.1.0 to proceed with this PR?

[tsegismont]

@tsegismont then if we want stick to behavior of java.nio.file.Files#readAllBytes, then should we move to the channel approach as you suggested or we can simply leave the current code that already use readAllBytes?

We should move to the channel approach but, for the error cases, we can reproduce the behavior of java.nio.file.Files#readAllBytes

[doxlik]

@tsegismont I published solution where tried to go FileChannel option but within error logic of readAllBytes. Check when you have time, please.

[tsegismont]

Thank you for the updates @doxlik

I've created #5730 , that supersedes this one.