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

IdleStateHandler create to many `IdleStateEvent` instances #973

Closed
shijinkui opened this Issue Jan 23, 2013 · 10 comments

Comments

Projects
None yet
4 participants
@shijinkui
Copy link

shijinkui commented Jan 23, 2013

class:IdleStateHandler, line: 396

channelIdle(ctx, new IdleStateEvent(
    IdleState.READER_IDLE, readerIdleCount ++, currentTime - lastReadTime));

i use this class to process read idle to send ping. every invoking generate a IdleStateEvent instance.
the result is 700000 connection send ping at the same time, after a moment, full gc forever.

i think we can improve it:

  1. evry channelIdle invoke should not create any instance. that can avoid too many full gc
  2. delay time point should distribute randomly time boundary, for example: 10~20 minute. that can avoid process idle event (send ping) at the same time.

@ghost ghost assigned normanmaurer Jan 23, 2013

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 23, 2013

@shijinkui I have something in mind.. let me implement it later today.

@ghost ghost assigned trustin Jan 23, 2013

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Jan 23, 2013

If we remove the 'duration' property from IdleStateEvent we can cache IdleStateEvent instances to reduce the frequency of instantiation dramatically.

Going even further, we can replace the 'count' property with a boolean property that tells if the event is the first idleness notification (i.e. count == 0).

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 23, 2013

@shijinkui fixed 1) ... Can you give more details about 2)

@shijinkui

This comment has been minimized.

Copy link

shijinkui commented Jan 24, 2013

start netty server --> request arrive at same time(10W a second for example), if we set the read idle time is 1 minute, that is at the next minute point there are 10W IdleStateHandler invoke to process at same time.

i have a simple implement. HeartBeatCheck(int readerIdleTime, int delayTime), the next readerIdleTime occur at a random time point at the range of delayTime. that is ervey IdleStateHandler distribute at ervey second randomly.

i think this can be a option method for netty user. At last I need it, LOL

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Jan 24, 2013

@normanmaurer, IIUC, he wants Netty to distribute the load of triggering a massive number of IdleStateEvents using a randomized variation in idle timeout value. For example, if a user specified 60 second reader idle time, IdleStateHandler could apply different idle time value for different connections within the user specified error range such as 1 second to apply 59.0~61.0 idle time.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 24, 2013

@trustin maybe we should make this configurable. As a user may want to have the "exact" timeout.

@ghost ghost assigned spullara Jan 24, 2013

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 25, 2013

@spullara you want to work in it ? Or are you assigned by mistake ?

@spullara

This comment has been minimized.

Copy link

spullara commented Jan 25, 2013

No idea how I got assigned.

All my photos are panoramas.

On Jan 25, 2013, at 2:31 AM, Norman Maurer notifications@github.com wrote:

@spullara https://github.com/spullara you want to work in it ? Or are you
assigned by mistake ?


Reply to this email directly or view it on
GitHubhttps://github.com//issues/973#issuecomment-12695516.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Jan 30, 2013

@trustin ok to close this ?

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Jan 30, 2013

Sure.

@trustin trustin closed this Jan 30, 2013

@ghost ghost assigned normanmaurer and trustin Jan 30, 2013

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