JAVA-2326: Reduce memory allocations during queries execution#1293
JAVA-2326: Reduce memory allocations during queries execution#1293netudima wants to merge 4 commits intoapache:3.xfrom
Conversation
… are evaluated for each request and allocate extra objects in heap but actually needed only for tracing. The values can be calculated lazy.
…ns, for Integer boxing within trace log parameters and for requests time tracking logic
adutra
left a comment
adutra
left a comment
There was a problem hiding this comment.
Thanks for you PR. Everything makes sense although I'm not sure any of these changes will produce a significant performance improvement. Have you benchmarked your changes? I'm also a bit reluctant in introducing these changes now that the 3.x branch is being retired.
|
|
||
| logger.trace("{}, stream {}, writing request {}", this, request.getStreamId(), request); | ||
| if (logger.isTraceEnabled()) | ||
| logger.trace("{}, stream {}, writing request {}", this, request.getStreamId(), request); |
There was a problem hiding this comment.
Just to make sure I understand, you are trying to avoid the array allocation for the varags arguments?
There was a problem hiding this comment.
array allocation in such places is usually optimized by escape analysis, the main issue which I see for such trace logging - hidden auto-boxing of int streamId.
The main goal of all the changes is to reduce an overall memory allocation on an application side (and to reduce GC pauses for the application correspondently). I am not expecting a visible change in request processing avg latency/throughput. Maybe some small CPU reduction will be as a bonus due to reducing some small amount of work.
Added a comment to https://datastax-oss.atlassian.net/browse/JAVA-2326
|
|
||
| if (doneWork) { | ||
| // Always flush what we have (don't artificially delay to try to coalesce more messages) | ||
| for (Channel channel : channels) channel.flush(); |
There was a problem hiding this comment.
Makes sense, although I'm not very comfortable changing this logic.
There was a problem hiding this comment.
the main trigger for the change is to avoid HashSet iterator objects creation for empty spin loops of flushing
|
|
||
| static EnumSet<Flag> deserialize(int flags) { | ||
| EnumSet<Flag> set = EnumSet.noneOf(Flag.class); | ||
| Flag[] values = Flag.values(); |
There was a problem hiding this comment.
Are you sure this gives us any perf improvement? I would expect the implementation of Enum.values() to be highly optimized.
There was a problem hiding this comment.
enum.values() method creates a new copy of array with values each time (due to inability to create immutable arrays in Java - https://dzone.com/articles/memory-hogging-enumvalues-method)
| private volatile List<Host> triedHosts; | ||
| private volatile ConcurrentMap<InetSocketAddress, Throwable> errors; | ||
|
|
||
| private final Timer.Context timerContext; |
There was a problem hiding this comment.
So you are just trying to avoid the allocation of a Timer.Context object, right?
There was a problem hiding this comment.
yes, trying to avoid the extra object allocation
| } | ||
|
|
||
| String getId() { | ||
| // atomicity is not required to set the value |
There was a problem hiding this comment.
This method could be called concurrently from different speculative executions. Are you saying atomicity is not required because even if 2 threads enter the method at the same time, the effect is going to be deterministic and idempotent, i.e. field id will be eventually set to Long.toString(System.identityHashCode(this))?
There was a problem hiding this comment.
yes, you are right, the result is deterministic and double evaluation is not a problem if it will happen (an atomic construction, like AtomicLong, would be more expensive here)
netudima
changed the title
|
Closing per my comment on the JIRA ticket. |
3 participants
Fields "id" are evaluated for each request and allocate extra objects in heap but actually needed only for tracing. The values can be calculated lazy.