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

Lazily initialize NativeDatagramPacketArray and IovArray in EpollEventLoop #8160

Merged
merged 1 commit into from Jul 29, 2018

Conversation

Projects
None yet
3 participants
@njhill
Contributor

njhill commented Jul 29, 2018

Motivation:

Avoid unnecessary native memory allocation if UDP / TCP isn't being used.

Modifications:

Create the reused NativeDatagramPacketArray and IovArray upon first use instead of EpollEventLoop construction. This more closely matches the prior FastThreadLocal.initialValue() behaviour changed in #8062.

Also correct related comment in NativeDatagramPacketArray.

Result:

Reduced native memory use in many cases when using epoll. Fixes #8159

iovArray.release();
datagramPacketArray.release();
if (iovArray != null) {
iovArray.release();

This comment has been minimized.

@normanmaurer

normanmaurer Jul 29, 2018

Member

can you also null out reference.

@normanmaurer

normanmaurer Jul 29, 2018

Member

can you also null out reference.

@normanmaurer

two small nits... funny enough I was just working on exactly the same you just finished it 2 mins earlier ;)

@normanmaurer normanmaurer self-assigned this Jul 29, 2018

@normanmaurer normanmaurer added this to the 4.1.29.Final milestone Jul 29, 2018

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 29, 2018

Member

Also please adjust the commit message:

Avoid unnecessary native memory allocation if UDP isn't being used.
Avoid unnecessary native memory allocation if UDP / TCP isn't being used.

Member

normanmaurer commented Jul 29, 2018

Also please adjust the commit message:

Avoid unnecessary native memory allocation if UDP isn't being used.
Avoid unnecessary native memory allocation if UDP / TCP isn't being used.

@njhill

This comment has been minimized.

Show comment
Hide comment
@njhill

njhill Jul 29, 2018

Contributor

Avoid unnecessary native memory allocation if UDP / TCP isn't being used.

Sure, I can change this. But if only UDP and not TCP is used then both datagramPacketArray and iovArray are still used. Is this what you meant? Or this is meant to refer to the case where the ELG is created but not all ELs have channels registered (which will also have memory saving here from fewer iovArrays)?

Contributor

njhill commented Jul 29, 2018

Avoid unnecessary native memory allocation if UDP / TCP isn't being used.

Sure, I can change this. But if only UDP and not TCP is used then both datagramPacketArray and iovArray are still used. Is this what you meant? Or this is meant to refer to the case where the ELG is created but not all ELs have channels registered (which will also have memory saving here from fewer iovArrays)?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 29, 2018

Member

Yeah if you use the group only for accepting new connections you save even more memory. Or if you only use it for UDP but not TCP you also save a bit.

Member

normanmaurer commented Jul 29, 2018

Yeah if you use the group only for accepting new connections you save even more memory. Or if you only use it for UDP but not TCP you also save a bit.

Lazy initialize NativeDatagramPacketArray and IovArray in EpollEventLoop
Motivation:

Avoid unnecessary native memory allocation if UDP / TCP isn't being
used.

Modifications:

Create the reused NativeDatagramPacketArray and IovArray upon first use
instead of EpollEventLoop construction.

Also correct related comment in NativeDatagramPacketArray.

Result:

Reduced native memory use when using epoll in many cases
@njhill

This comment has been minimized.

Show comment
Hide comment
@njhill

njhill Jul 29, 2018

Contributor

Thanks @normanmaurer, I updated the commit with requested wording and ref null-ing.

Contributor

njhill commented Jul 29, 2018

Thanks @normanmaurer, I updated the commit with requested wording and ref null-ing.

@normanmaurer normanmaurer merged commit 630c827 into netty:4.1 Jul 29, 2018

1 check failed

continuous-integration/teamcity Finished TeamCity Build pull requests :: netty : Tests failed: 1, passed: 13848, ignored: 127
Details

@njhill njhill deleted the njhill:lazy-epoll-mem branch Jul 29, 2018

@njhill

This comment has been minimized.

Show comment
Hide comment
@njhill

njhill Jul 29, 2018

Contributor

@normanmaurer do you think it would be worthwhile to do the same thing with

channels = new IntObjectHashMap<AbstractEpollChannel>(4096);

to save a bit of heap given how large the init capacity is? If so I can open another PR.

Contributor

njhill commented Jul 29, 2018

@normanmaurer do you think it would be worthwhile to do the same thing with

channels = new IntObjectHashMap<AbstractEpollChannel>(4096);

to save a bit of heap given how large the init capacity is? If so I can open another PR.

@johnou

This comment has been minimized.

Show comment
Hide comment
@johnou

johnou Jul 30, 2018

Contributor

@normanmaurer this hit us pretty bad, when do you expect to release 4.1.29.Final?

Contributor

johnou commented Jul 30, 2018

@normanmaurer this hit us pretty bad, when do you expect to release 4.1.29.Final?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Jul 30, 2018

Member

@njhill I don't think this is really needed imho as we always need the map anyway (if you will ever register a Channel).

@johnou within the next two weeks.

Member

normanmaurer commented Jul 30, 2018

@njhill I don't think this is really needed imho as we always need the map anyway (if you will ever register a Channel).

@johnou within the next two weeks.

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