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

Improve new DefaultChannelId #13960

Merged
merged 1 commit into from Apr 10, 2024
Merged

Improve new DefaultChannelId #13960

merged 1 commit into from Apr 10, 2024

Conversation

jchrys
Copy link
Contributor

@jchrys jchrys commented Apr 9, 2024

Motivation:
The instantiation process of DefaultChannelId presents opportunities for optimization.

Modifications:
Use the local stack when creating an instance.
Employ Unsafe.

Result:
Enhanced Performance.

@jchrys
Copy link
Contributor Author

jchrys commented Apr 9, 2024

1X10X2, Intel(R) Xeon(R) Silver 4210 CPU @ 2.20GHz, openjdk 17.0.8 2023-07-18, Ubuntu 22.04.3 LTS, tuend network low-latency, no turbo boost.

benchmark result

@jchrys jchrys marked this pull request as draft April 9, 2024 14:56
Motivation:
The instantiation process of DefaultChannelId presents opportunities for optimization.

Modifications:
Use the local stack when creating an instance.
Employ `Unsafe`.

Result:
Enhanced Performance.
@jchrys
Copy link
Contributor Author

jchrys commented Apr 9, 2024

1X10X2, Intel(R) Xeon(R) Silver 4210 CPU @ 2.20GHz, openjdk 17.0.8 2023-07-18, Ubuntu 22.04.3 LTS, tuend network low-latency, no turbo boost.

latest benchmark result

@jchrys jchrys marked this pull request as ready for review April 9, 2024 15:08
@jchrys jchrys requested a review from franz1981 April 9, 2024 15:26
Copy link
Contributor

@chrisvest chrisvest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Netty 5 I suspect we'll be better off to just assign instance fields (and bump the serialVersionUID) instead of encoding a byte array.

@jchrys
Copy link
Contributor Author

jchrys commented Apr 9, 2024

In Netty 5 I suspect we'll be better off to just assign instance fields (and bump the serialVersionUID) instead of encoding a byte array.

I agree. That approach would be straightforward and could save resources by avoiding the assignment & calculation of constant values such as MACINE_ID and PROCESS_ID as well.

@chrisvest
Copy link
Contributor

That approach would be straightforward and could save resources by avoiding the assignment & calculation of constant values such as MACINE_ID and PROCESS_ID as well.

There still needs to be instance fields for those, because ChannelId is Serializable, but they're just plain field assignments.

@jchrys
Copy link
Contributor Author

jchrys commented Apr 9, 2024

There still needs to be instance fields for those, because ChannelId is Serializable, but they're just plain field assignments.

Thank you for clarifying that for me 👍

@jchrys
Copy link
Contributor Author

jchrys commented Apr 9, 2024

@chrisvest
May I submit a PR for Netty5 based on the information above?

@chrisvest
Copy link
Contributor

@jchrys Yes, please go ahead.

@chrisvest chrisvest merged commit e19c91f into netty:4.1 Apr 10, 2024
16 checks passed
@jchrys jchrys deleted the 4.1-enhance-cid branch April 11, 2024 00:14
@jchrys jchrys mentioned this pull request Apr 11, 2024
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 this pull request may close these issues.

None yet

3 participants