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

#1422 follow up, Reuse ChannelRequest and ChannelNotification #1423

Closed
wants to merge 1 commit into from

Conversation

Lynnworld
Copy link
Contributor

Try reuse ChannelRequest and ChannelNotification , and no more thread_local static flatbuffer builder.

@jmillan
Copy link
Member

jmillan commented Jul 4, 2024

I'll be few days away from the computer. Let me review this once I'm back after the weekend.

@jmillan
Copy link
Member

jmillan commented Jul 9, 2024

I don't like this approach. Each incoming request needs to create a new Request instance. I don't like the fact that we reuse the same request all over.

If you are doing this for performance purposes, please show us the penalties of creating a Request instance for each incoming request. We can sacrifice design for performance up to a point, and IMHO this is not justified.

@jmillan jmillan closed this Jul 9, 2024
@ibc
Copy link
Member

ibc commented Jul 9, 2024

I don't like this approach. Each incoming request needs to create a new Request instance. I don't like the fact that we reuse the same request all over.

If you are doing this for performance purposes, please show us the penalties of creating a Request instance for each incoming request. We can sacrifice design for performance up to a point, and IMHO this is not justified.

Agreed. Definitely having this->request.Receive(xxx) is not nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants