Skip to content

Improve new DefaultChannelId#13960

Merged
chrisvest merged 1 commit intonetty:4.1from
jchrys:4.1-enhance-cid
Apr 10, 2024
Merged

Improve new DefaultChannelId#13960
chrisvest merged 1 commit intonetty:4.1from
jchrys:4.1-enhance-cid

Conversation

@jchrys
Copy link
Copy Markdown
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 jchrys force-pushed the 4.1-enhance-cid branch from 5171232 to 55f6618 Compare April 9, 2024 14:16
@jchrys
Copy link
Copy Markdown
Contributor Author

jchrys commented Apr 9, 2024 •
edited
Loading

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

franz1981
franz1981 reviewed Apr 9, 2024
View reviewed changes
Comment thread transport/src/main/java/io/netty/channel/DefaultChannelId.java Outdated
@jchrys jchrys marked this pull request as draft April 9, 2024 14:56
@jchrys
Improve new DefaultChannelId
1b66bf5 
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 jchrys force-pushed the 4.1-enhance-cid branch from 55f6618 to 1b66bf5 Compare April 9, 2024 15:00
@jchrys
Copy link
Copy Markdown
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
chrisvest
chrisvest approved these changes Apr 9, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@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
Copy Markdown
Contributor Author

jchrys commented Apr 9, 2024 •
edited
Loading

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
Copy Markdown
Member

chrisvest commented Apr 9, 2024

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
Copy Markdown
Contributor Author

jchrys commented Apr 9, 2024 •
edited
Loading

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
Copy Markdown
Contributor Author

jchrys commented Apr 9, 2024

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

@chrisvest
Copy link
Copy Markdown
Member

chrisvest commented Apr 9, 2024

@jchrys Yes, please go ahead.

@chrisvest chrisvest merged commit e19c91f into netty:4.1 Apr 10, 2024
@jchrys jchrys deleted the 4.1-enhance-cid branch April 11, 2024 00:14
@jchrys jchrys mentioned this pull request Apr 11, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chrisvest chrisvest chrisvest approved these changes

@franz1981 franz1981 Awaiting requested review from franz1981

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jchrys @chrisvest @franz1981