Add 'toString' method into IdleStateEvent #9695
Conversation
|
Can one of the admins verify this patch? |
| @@ -31,6 +32,7 @@ | |||
|
|
|||
| private final IdleState state; | |||
| private final boolean first; | |||
| private String strVal; | |||
There was a problem hiding this comment.
You probably don't need to cache this, since it's unlikely to be reused.
There was a problem hiding this comment.
I agree with @carl-mastrangelo ... @ursaj can you remove this ?
There was a problem hiding this comment.
you both are mistaken in the way, how this class is used. there are only 6 shared (re-usable) instances of this class in runtime:
public class IdleStateEvent {
public static final IdleStateEvent FIRST_READER_IDLE_STATE_EVENT = new IdleStateEvent(IdleState.READER_IDLE, true);
public static final IdleStateEvent READER_IDLE_STATE_EVENT = new IdleStateEvent(IdleState.READER_IDLE, false);
public static final IdleStateEvent FIRST_WRITER_IDLE_STATE_EVENT = new IdleStateEvent(IdleState.WRITER_IDLE, true);
public static final IdleStateEvent WRITER_IDLE_STATE_EVENT = new IdleStateEvent(IdleState.WRITER_IDLE, false);
public static final IdleStateEvent FIRST_ALL_IDLE_STATE_EVENT = new IdleStateEvent(IdleState.ALL_IDLE, true);
public static final IdleStateEvent ALL_IDLE_STATE_EVENT = new IdleStateEvent(IdleState.ALL_IDLE, false);
So cached string value is used countless times.
There was a problem hiding this comment.
The same applies to inherited instances (if any):
- either they are used in the same shared model, so this optimisation is useful;
- or they are created, used and GC-ed in a short while, so there is no harm in this optimisation.
There was a problem hiding this comment.
I disagree here. If it is the case that the same idle events are reused heavily, then it stands to reason that they are shared by multiple event loops. That makes this code racy and will trigger the race detector.
There was a problem hiding this comment.
@carl-mastrangelo generate strings on each invocation doesn't look good either. any ideas or examples how it should look like?
There was a problem hiding this comment.
ah, I see. you've already tried to fix it in PR #9705
| @@ -31,6 +32,7 @@ | |||
|
|
|||
| private final IdleState state; | |||
| private final boolean first; | |||
| private String strVal; | |||
There was a problem hiding this comment.
I agree with @carl-mastrangelo ... @ursaj can you remove this ?
|
@netty-bot test this please |
|
@netty-bot test this please |
|
@netty-bot test this please |
|
@ursaj thanks a lot! |
IdleStateEvent is very convenient and frequently used type of events. However both in runtime (logs) and in debug you need some manual steps to see their actual content. Default implementation generates worthless trash like this:
io.netty.handler.timeout.IdleStateEvent@27f674d
There are examples already, where event has convenient and useful toString implementation:
* io.netty.handler.proxy.ProxyConnectionEvent
* io.netty.handler.ssl.SslCompletionEvent
* Implement 'IdleStateEvent.toString' method.
* Unit test.
More useful String representation of IdleStateEvent
Motivation: In PR netty#9695 IdleStateEvents were made to cache their string representation. The reason for this was to avoid creating garbage as these values would be used frequently. However, these objects may be used on multiple event loops and this may cause an unexpected race to occur. Modification: Only make the events that Netty creates cache their toString representation. Result: No races.
Motivation: In PR #9695 IdleStateEvents were made to cache their string representation. The reason for this was to avoid creating garbage as these values would be used frequently. However, these objects may be used on multiple event loops and this may cause an unexpected race to occur. Modification: Only make the events that Netty creates cache their toString representation. Result: No races.
Motivation: In PR #9695 IdleStateEvents were made to cache their string representation. The reason for this was to avoid creating garbage as these values would be used frequently. However, these objects may be used on multiple event loops and this may cause an unexpected race to occur. Modification: Only make the events that Netty creates cache their toString representation. Result: No races.
Motivation:
IdleStateEvent is very convenient and frequently used type of events. However both in runtime (logs) and in debug you need some manual steps to see their actual content. Default implementation generates worthless trash like this:
There are examples already, where event has convenient and useful toString implementation:
Modification: