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

Can you provide an interface that reads at least n bytes and reads as many as possible? #198

Closed
jiangdongzi opened this issue Oct 18, 2023 · 11 comments · Fixed by #224
Closed

Comments

@jiangdongzi
Copy link

image
This way you may decrease system calls. Like the image points out, if read not just header, then may decrease system call.

@lihuiba
Copy link
Collaborator

lihuiba commented Oct 19, 2023

How do you define "as many as possible"?

@beef9999
Copy link
Collaborator

ISocketStream::recv equals to libc recv, it may read less than the number you desired, if the network buffer is drained. ISocketStream::read equals to fully_recv, it will read the exact number of bytes, probably waiting.
https://photonlibos.github.io/docs/api/network#isocketstream

@jiangdongzi
Copy link
Author

How do you define "as many as possible"?

such as set recv count 4k.

@jiangdongzi
Copy link
Author

ISocketStream::recv equals to libc recv, it may read less than the number you desired, if the network buffer is drained. ISocketStream::read equals to fully_recv, it will read the exact number of bytes, probably waiting. https://photonlibos.github.io/docs/api/network#isocketstream

I read some of the source code, I know thre "read" is atomic and how "atomic" is implemented. So we can use the same implementation, if read count less than "the least count", then we set the thread sleep and wait fd readable. But there's no need we limit the count to "least count". This way we may read not only the header, but also the body. Or even multiple requests.

@lihuiba
Copy link
Collaborator

lihuiba commented Oct 19, 2023

@jiangdongzi Is this close to what you want?

size_t recv_at_least(void* buf, size_t size, size_t least) {
    size_t count = 0;
    least = min(least, size);
    while (true) {
        ssize_t ret = this->recv(buf, size);
        if (ret <= 0) {...}
        if ((count += ret) >= least) break;
        size -= ret;
    }
    return count;
}

@lihuiba
Copy link
Collaborator

lihuiba commented Oct 19, 2023

or even, we can read as many as possible:

A single syscall already recvs as many bytes as possible from kernel socket buffer, but not exceeding size. The while loop is to ensure at least that many bytes. Is this what you meant?

@jiangdongzi
Copy link
Author

or even, we can read as many as possible:

A single syscall already recvs as many bytes as possible from kernel socket buffer, but not exceeding size. The while loop is to ensure at least that many bytes. Is this what you meant?

Yeah, you are right, there's no need multiple system calls, your code above does the thing.
Anyway, my point is that we need read as many as possible, not just read the 'header' and then the 'body'. If multiple requests come, we need read header, then body, then header, then body...in current implementation.

@lihuiba
Copy link
Collaborator

lihuiba commented Oct 19, 2023

Yes, in more general cases, it can be beneficial to recv as much data as possible, so as to reduce the number of syscalls.

But in this specific case of rpc, as shown in your screenshot, we have to recv the header first, so that we know the actual size of the body, and allocate a buffer for receiving the body.

@jiangdongzi
Copy link
Author

Yes, in more general cases, it can be beneficial to recv as much data as possible, so as to reduce the number of syscalls.

But in this specific case of rpc, as shown in your screenshot, we have to recv the header first, so that we know the actual size of the body, and allocate a buffer for receiving the body.

RPC is just the general case, I think body is just behind header is the most case. We can allocate a relatively large buffer that probally bigger than the (header + body) or even larger that can accommodate several requests. I have read brpc code, it does so.

@lihuiba
Copy link
Collaborator

lihuiba commented Oct 20, 2023

By "rpc", I meant the specific one shown in your screenshot.

Your suggestion for recv (and rpc) makes sense, and we'll add it to photon.

@lihuiba
Copy link
Collaborator

lihuiba commented Oct 20, 2023

Thanks!

@lihuiba lihuiba linked a pull request Oct 28, 2023 that will close this issue
@lihuiba lihuiba closed this as completed Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants