Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AbstractIOUringStreamChannel iovArray litter #154

Open
pveentjer opened this issue Apr 12, 2022 · 3 comments
Open

AbstractIOUringStreamChannel iovArray litter #154

pveentjer opened this issue Apr 12, 2022 · 3 comments
Labels
enhancement New feature or request

Comments

@pveentjer
Copy link
Contributor

pveentjer commented Apr 12, 2022

Every time there is a writev, an IovArray instance is created. Also, there is the overhead of pooling the memory of the iovArrayBuffer.

 @Override
        protected int scheduleWriteMultiple(ChannelOutboundBuffer in) {
            assert iovArray == null;
            int numElements = Math.min(in.size(), Limits.IOV_MAX);
            ByteBuf iovArrayBuffer = alloc().directBuffer(numElements * IovArray.IOV_SIZE);
            iovArray = new IovArray(iovArrayBuffer); <-----
            try {
                int offset = iovArray.count();
                in.forEachFlushedMessage(iovArray);
                submissionQueue().addWritev(socket.intValue(),
                        iovArray.memoryAddress(offset), iovArray.count() - offset, (short) 0);
            } catch (Exception e) {
                // This should never happen, anyway fallback to single write.
                iovArray.release();
                iovArray = null;
                scheduleWriteSingle(in.current());
            }
            return 1;
        }

Perhaps it would be better to preallocate the iovArray with ByteBuff of size:

ByteBuf iovArrayBuffer = alloc().directBuffer(IOV_MAX * IovArray.IOV_SIZE);
iovArray = new IovArray(iovArrayBuffer); 

This way the iovArray is always big enough for any number of buffers.

So the usage would look something like this:

 @Override
        protected int scheduleWriteMultiple(ChannelOutboundBuffer in) {
            iovArray.clear();
            try {
                int offset = iovArray.count();
                in.forEachFlushedMessage(iovArray);
                submissionQueue().addWritev(socket.intValue(),
                        iovArray.memoryAddress(offset), iovArray.count() - offset, (short) 0);
            } catch (Exception e) {
                // This should never happen, anyway fallback to single write.
                scheduleWriteSingle(in.current());
            }
            return 1;
        }

One drawback of preallocating is that it will consume extra memory. So with a 64 bit JVM and 1024 as IOV_MAX, this would be around '2 * 8B * 1024 = 16KB'. So perhaps that would be a reason to not pool by default.

@franz1981
Copy link
Contributor

I would use unpooled direct memory there too, and yes, agree on your point

@1Jo1 any concern why the iov array was treated as immutable and allocated/released for each writev?
I remember that we cannot have more the one in-flight write (vectored or not), hence it shouldn't happen that iovArray will be used while its memory still used for a previously submitted (and never completed) one

@normanmaurer
Copy link
Member

Yes I think we can pre-allocate.

@franz1981 franz1981 added the enhancement New feature or request label Apr 19, 2022
@franz1981
Copy link
Contributor

@pveentjer do you think you can send a PR? I can help if needed, obviously :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants