[
https://issues.apache.org/jira/browse/CALCITE-3325?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16924634#comment-16924634
]
Mehdi Salarkia commented on CALCITE-3325:
-----------------------------------------
Thanks [~elserj] for your quick response and looking into this.
I think I have to first explain the test environment so it would be clear. We
have a set of Phoenix query severs and a set of clients talking to the query
servers via protobuf protocol and Avatica.
The client code runs on jetty which has a fixed thread pool for serving the
requests. We use a client side connection pool Avatica connections. The
connections could be recycled and used between threads but the connection pool
could also create/drop connections as necessary.
What we have observed was a excessive garbage collection when service is under
load for a some time. From what we can see is there are a lot of
UnsynchronizedBuffer instances for each thread (please see the two attached
snapshots) which shouldn't be the case if they were suppose to be recycled and
reused by same thread. You are right that the size of these object is
insignificant but that's because garbage collection has already cleaned up many
of the instances and needs to constantly do it in order that application can
maintain its memory. From what we can see right now we have a high CPU
utilization due to this issue.
>From my understanding objects stored in ThreadLocal are not removed until a
>garbage collection happens due to insufficient memory, see in
>java.lang.ThreadLocal.ThreadLocalMap:
{code:java}
/**
* ThreadLocalMap is a customized hash map suitable only for
* maintaining thread local values. No operations are exported
* outside of the ThreadLocal class. The class is package private to
* allow declaration of fields in class Thread. To help deal with
* very large and long-lived usages, the hash table entries use
* WeakReferences for keys. However, since reference queues are not
* used, stale entries are guaranteed to be removed only when
* the table starts running out of space.
*/
static class ThreadLocalMap
{code}
I wrote a test case to explain the scenario :
{code:java}
import java.util.Random;
import java.util.UUID;
public class ThreadLocalTest {
private final ThreadLocal<UnsynchronizedBuffer> THREAD_LOCAL_BUFFER =
new ThreadLocal<UnsynchronizedBuffer>() {
@Override
protected UnsynchronizedBuffer initialValue() {
return new UnsynchronizedBuffer();
}
};
public static void main(String[] args) throws Exception {
Thread[] threads = new Thread[10];
for (int i = 0; i < threads.length; i++) {
threads[i] = new Thread(new Runnable() {
@Override
public void run() {
while (true) {
ThreadLocalTest test = new ThreadLocalTest();
UnsynchronizedBuffer buffer =
test.THREAD_LOCAL_BUFFER.get();
buffer.doSomething();
Thread.yield();
try {
Thread.sleep(10);
} catch (Exception e) {
e.printStackTrace();
}
}
}
});
threads[i].start();
}
for (Thread thread : threads) {
thread.join();
}
}
static class UnsynchronizedBuffer {
byte[] data = new byte[100000];
@Override
protected void finalize() throws Throwable {
super.finalize();
System.out.println("Clean up");
}
public void doSomething() {
}
}
}
{code}
And to make it to show the impact, I ran it with the following configuration
{code:java}
-Xms10m -Xmx10m
{code}
and captured two snapshots with Yourkit one for `THREAD_LOCAL_BUFFER` defined
as `non static` and the other one as `static` (Please see
attached[^Non-Static.snapshot] & Static.snapshot).
If you look at the garbage collection metrics for the two snapshots for GC
metrics (Non-Static.png & Static.png) You could see the impact I'm talking
about.
Also by simply running the code above you will see the console will quickly
start to show the "Clean up" message which shouldn't be the case as the design
that you had was to be able to re-use the object stored in threadLocal but what
is happening in reality is the UnsynchronizedBuffer is deleted and recreated
for each time any of the threads is trying to use it.
Hope this explains the problem in more details.
Thanks.
> Thread Local Buffer Variable (threadLocalBuffer) In ProtobufTranslationImpl
> Is Defined As Non Static Causing Memory Leak
> ------------------------------------------------------------------------------------------------------------------------
>
> Key: CALCITE-3325
> URL: https://issues.apache.org/jira/browse/CALCITE-3325
> Project: Calcite
> Issue Type: Bug
> Reporter: Mehdi Salarkia
> Priority: Major
> Attachments: Non-Static.snapshot, Non-static.png, Screen Shot
> 2019-09-05 at 5.05.20 PM.png, Screen Shot 2019-09-05 at 5.18.19 PM.png,
> Static.png, Static.snapshot
>
>
> As we were load testing our system on Apache Phoenix via the thin client
> which uses Avatica we ran into Garbage collection problems. After some
> investigation we could see there are a lot of unreferenced object due to this
> variable:
> {code:java}
> private final ThreadLocal<UnsynchronizedBuffer> threadLocalBuffer =
> new ThreadLocal<UnsynchronizedBuffer>() {
> @Override protected UnsynchronizedBuffer initialValue() {
> return new UnsynchronizedBuffer();
> }
> };
> {code}
> Which seems to be a reusable buffer for serializing/deserializing the data.
> From my understating there is a copy of this variable created per each
> instance of ProtobufTranslationImpl. However the proper use of ThreadLocal it
> seems to be one instance per thread and the current implementation seems to
> be missing the `static` keyword when defining the thread local variable:
> See [https://docs.oracle.com/javase/7/docs/api/java/lang/ThreadLocal.html]
> {code:java}
> ThreadLocal instances are typically private static fields in classes that
> wish to associate state with a thread (e.g., a user ID or Transaction ID).
> {code}
> See the attached snapshot from a memory dump we took.
--
This message was sent by Atlassian Jira
(v8.3.2#803003)
