Re: Class field ThreadLocal. Why not static?
Dmitry, Thanks for your example! The approach looks similar to "problematic" on used in jdk described in [1]. Fortunately, in our case buffer is quite limited. On the other hand we always should be cautious because Ignite code frequently is running on user threads (e.g. it is typical for SQL). [1] http://www.evanjones.ca/java-bytebuffer-leak.html 2018-09-13 9:28 GMT+03:00 Alexey Goncharuk : > Maxim, > > If multiple instances of Ignite is started in the same JVM and a user > thread will access first one instance of Ignite, then another, you will end > up with the static thread local holding the last WAL pointer from the > second grid. This is possible, for example, when a user thread commits a > transaction or runs an atomic update on a data node. Any access of the > first Ignite instance will have an invalid thread-local value. > > вт, 11 сент. 2018 г. в 13:29, Maxim Muzafarov : > > > Alexey, Ivan, > > > > Agree. Keeping strong references to the Thread object is the source of > > memory leak with ThreadLocals variables > > and the values that it stores. ThreadLocalMap is bound to the Thread > > lifespan [1], so I think when we are using > > everything right all will be GC'ed correctly. > > Is this memory leaks with ThreadLocal's you mean, Alexey? If not, please, > > share your example. > > > > Also, agree that these usages should be bound to the component lifespan. > > But for `FileWriteAheadLogManager` > > I think this variable used not semantically right. I've dumped all > threads > > (total ~49 threads) > > that are using `lastWalPtr` in `FileWriteAheadLogManager`. For instance, > > * exchange-worker-#40%wal.IgniteWalRecoveryTest0% > > * sys-#148%wal.IgniteWalRecoveryTest1% > > * db-checkpoint-thread-#129%wal.IgniteWalRecoveryTest2% > > Suppose everything would be OK here for `static` and `non-static` case of > > ThreadLocal. > > > > [1] > > > > http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/ > share/classes/java/lang/Thread.java#l760 > > > > On Tue, 11 Sep 2018 at 13:05 Павлухин Иван wrote: > > > > > Dmitriy, > > > > > > Could you point to some piece of code implementing described pattern? > > > > > > 2018-09-11 13:02 GMT+03:00 Павлухин Иван : > > > > > > > Alex, > > > > > > > > ThreadLocal subclass is used in IgniteH2Indexing for simple access to > > H2 > > > > Connection from current thread. Such subclass has a capability to > > create > > > > connection if one does not exist, so obtaining connection is merely > > > > ThreadLocal.get. Also there are scheduled routines to cleanup > > connections > > > > and associated with them statement cache after some expiration time. > > For > > > > that reason Map is maintained. As query > > can > > > > run on user thread we need to cleanup mentioned map to avoid a leak > > when > > > > Thread is terminated. So we need to check thread status in cleanup > > > routines > > > > and remove entries for terminated Threads. And historically there was > > no > > > > cleanup for terminated threads and leak was possible. And also great > > care > > > > must be taken in order to avoid cyclic reference between ThreadLocal > > > > instance and a stored value. Which easily could occur if the stored > > value > > > > is covered by multiple layers of abstraction. > > > > > > > > And I am describing some historical state. Now machinery in > > > IgniteH2Indexing > > > > is even more complex (I hope we will have a chance to improve it). > > > > > > > > 2018-09-11 11:00 GMT+03:00 Alexey Goncharuk < > > alexey.goncha...@gmail.com > > > >: > > > > > > > >> Ivan, > > > >> > > > >> Can you elaborate on the issue with the thread local cleanup you've > > > faced? > > > >> > > > >> вт, 11 сент. 2018 г. в 9:13, Павлухин Иван : > > > >> > > > >> > Guys, > > > >> > > > > >> > As we know ThreadLocal is an instrument which should be used with > > > great > > > >> > care. And I recently faced with problems related to proper cleanup > > of > > > >> > ThreadLocal which is not needed anymore. In my opinion the best > > thing > > > >> (in > > > >> > ideal world) is to get rid of ThreadLocal where possible, but I > > guess > > > >> that > > > >> > it is quite hard (in real world). > > > >> > > > > >> > Also, a question comes to my mind. As ThreadLocal is so common in > > our > > > >> code, > > > >> > could you suggest some guidance or code fragments which address > > proper > > > >> > ThreadLocal > > > >> > lifecycle control and especially cleanup? > > > >> > > > > >> > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk < > > > alexey.goncha...@gmail.com > > > >> >: > > > >> > > > > >> > > Maxim, > > > >> > > > > > >> > > Ignite supports starting multiple instances of Ignite in the > same > > > VM, > > > >> so > > > >> > > having static thread locals for the fields you mentioned does > not > > > >> work. > > > >> > > > > > >> > > Generally, I think thread-local should be bound to the lifespan > of > > > the > > > >> > > component it describes. Static thread-locals are hard to > clean-up > >
Re: Class field ThreadLocal. Why not static?
Maxim, If multiple instances of Ignite is started in the same JVM and a user thread will access first one instance of Ignite, then another, you will end up with the static thread local holding the last WAL pointer from the second grid. This is possible, for example, when a user thread commits a transaction or runs an atomic update on a data node. Any access of the first Ignite instance will have an invalid thread-local value. вт, 11 сент. 2018 г. в 13:29, Maxim Muzafarov : > Alexey, Ivan, > > Agree. Keeping strong references to the Thread object is the source of > memory leak with ThreadLocals variables > and the values that it stores. ThreadLocalMap is bound to the Thread > lifespan [1], so I think when we are using > everything right all will be GC'ed correctly. > Is this memory leaks with ThreadLocal's you mean, Alexey? If not, please, > share your example. > > Also, agree that these usages should be bound to the component lifespan. > But for `FileWriteAheadLogManager` > I think this variable used not semantically right. I've dumped all threads > (total ~49 threads) > that are using `lastWalPtr` in `FileWriteAheadLogManager`. For instance, > * exchange-worker-#40%wal.IgniteWalRecoveryTest0% > * sys-#148%wal.IgniteWalRecoveryTest1% > * db-checkpoint-thread-#129%wal.IgniteWalRecoveryTest2% > Suppose everything would be OK here for `static` and `non-static` case of > ThreadLocal. > > [1] > > http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/Thread.java#l760 > > On Tue, 11 Sep 2018 at 13:05 Павлухин Иван wrote: > > > Dmitriy, > > > > Could you point to some piece of code implementing described pattern? > > > > 2018-09-11 13:02 GMT+03:00 Павлухин Иван : > > > > > Alex, > > > > > > ThreadLocal subclass is used in IgniteH2Indexing for simple access to > H2 > > > Connection from current thread. Such subclass has a capability to > create > > > connection if one does not exist, so obtaining connection is merely > > > ThreadLocal.get. Also there are scheduled routines to cleanup > connections > > > and associated with them statement cache after some expiration time. > For > > > that reason Map is maintained. As query > can > > > run on user thread we need to cleanup mentioned map to avoid a leak > when > > > Thread is terminated. So we need to check thread status in cleanup > > routines > > > and remove entries for terminated Threads. And historically there was > no > > > cleanup for terminated threads and leak was possible. And also great > care > > > must be taken in order to avoid cyclic reference between ThreadLocal > > > instance and a stored value. Which easily could occur if the stored > value > > > is covered by multiple layers of abstraction. > > > > > > And I am describing some historical state. Now machinery in > > IgniteH2Indexing > > > is even more complex (I hope we will have a chance to improve it). > > > > > > 2018-09-11 11:00 GMT+03:00 Alexey Goncharuk < > alexey.goncha...@gmail.com > > >: > > > > > >> Ivan, > > >> > > >> Can you elaborate on the issue with the thread local cleanup you've > > faced? > > >> > > >> вт, 11 сент. 2018 г. в 9:13, Павлухин Иван : > > >> > > >> > Guys, > > >> > > > >> > As we know ThreadLocal is an instrument which should be used with > > great > > >> > care. And I recently faced with problems related to proper cleanup > of > > >> > ThreadLocal which is not needed anymore. In my opinion the best > thing > > >> (in > > >> > ideal world) is to get rid of ThreadLocal where possible, but I > guess > > >> that > > >> > it is quite hard (in real world). > > >> > > > >> > Also, a question comes to my mind. As ThreadLocal is so common in > our > > >> code, > > >> > could you suggest some guidance or code fragments which address > proper > > >> > ThreadLocal > > >> > lifecycle control and especially cleanup? > > >> > > > >> > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk < > > alexey.goncha...@gmail.com > > >> >: > > >> > > > >> > > Maxim, > > >> > > > > >> > > Ignite supports starting multiple instances of Ignite in the same > > VM, > > >> so > > >> > > having static thread locals for the fields you mentioned does not > > >> work. > > >> > > > > >> > > Generally, I think thread-local should be bound to the lifespan of > > the > > >> > > component it describes. Static thread-locals are hard to clean-up > > and > > >> > they > > >> > > often lead to leaks, so I would rather changed existing static > > >> > > thread-locals to be non-static. > > >> > > > > >> > > --AG > > >> > > > > >> > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov >: > > >> > > > > >> > > > Igniters, > > >> > > > > > >> > > > According to javadoc [1] class ThreadLocal: > > >> > > > `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).` > > >> > > > > > >> > > > So, AFAIK non-static ThreadLocal usage means as `per thread - > per > > >> cla
Re: Class field ThreadLocal. Why not static?
Hi Ivan, For example, I remember thread local buffer in our old WAL implementation: org.apache.ignite.internal.processors.cache.persistence.wal.FsyncModeFileWriteAheadLogManager#tlb Sincerely, Dmitriy Pavlov вт, 11 сент. 2018 г. в 13:05, Павлухин Иван : > Dmitriy, > > Could you point to some piece of code implementing described pattern? > > 2018-09-11 13:02 GMT+03:00 Павлухин Иван : > > > Alex, > > > > ThreadLocal subclass is used in IgniteH2Indexing for simple access to H2 > > Connection from current thread. Such subclass has a capability to create > > connection if one does not exist, so obtaining connection is merely > > ThreadLocal.get. Also there are scheduled routines to cleanup connections > > and associated with them statement cache after some expiration time. For > > that reason Map is maintained. As query can > > run on user thread we need to cleanup mentioned map to avoid a leak when > > Thread is terminated. So we need to check thread status in cleanup > routines > > and remove entries for terminated Threads. And historically there was no > > cleanup for terminated threads and leak was possible. And also great care > > must be taken in order to avoid cyclic reference between ThreadLocal > > instance and a stored value. Which easily could occur if the stored value > > is covered by multiple layers of abstraction. > > > > And I am describing some historical state. Now machinery in > IgniteH2Indexing > > is even more complex (I hope we will have a chance to improve it). > > > > 2018-09-11 11:00 GMT+03:00 Alexey Goncharuk >: > > > >> Ivan, > >> > >> Can you elaborate on the issue with the thread local cleanup you've > faced? > >> > >> вт, 11 сент. 2018 г. в 9:13, Павлухин Иван : > >> > >> > Guys, > >> > > >> > As we know ThreadLocal is an instrument which should be used with > great > >> > care. And I recently faced with problems related to proper cleanup of > >> > ThreadLocal which is not needed anymore. In my opinion the best thing > >> (in > >> > ideal world) is to get rid of ThreadLocal where possible, but I guess > >> that > >> > it is quite hard (in real world). > >> > > >> > Also, a question comes to my mind. As ThreadLocal is so common in our > >> code, > >> > could you suggest some guidance or code fragments which address proper > >> > ThreadLocal > >> > lifecycle control and especially cleanup? > >> > > >> > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk < > alexey.goncha...@gmail.com > >> >: > >> > > >> > > Maxim, > >> > > > >> > > Ignite supports starting multiple instances of Ignite in the same > VM, > >> so > >> > > having static thread locals for the fields you mentioned does not > >> work. > >> > > > >> > > Generally, I think thread-local should be bound to the lifespan of > the > >> > > component it describes. Static thread-locals are hard to clean-up > and > >> > they > >> > > often lead to leaks, so I would rather changed existing static > >> > > thread-locals to be non-static. > >> > > > >> > > --AG > >> > > > >> > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov : > >> > > > >> > > > Igniters, > >> > > > > >> > > > According to javadoc [1] class ThreadLocal: > >> > > > `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).` > >> > > > > >> > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per > >> class > >> > > > instance`. What the real cases of using non-static ThreadLocal > class > >> > > fields > >> > > > in Ignite code project? When we need it? > >> > > > > >> > > > In Ignite code project I've found ThreadLocal usage as: > >> > > > - non-static - 67 > >> > > > - static - 68 > >> > > > > >> > > > Back to my example, I've checked FileWriteAheadLogManager. It has: > >> > > > 1) private final ThreadLocal interrupted [2] > >> > > > 2) private final ThreadLocal lastWALPtr [3] > >> > > > I think both of these fields should be set and used as `static`. > Can > >> > > anyone > >> > > > confirm it? > >> > > > > >> > > > > >> > > > [1] > >> > https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html > >> > > > [2] > >> > > > > >> > > > https://github.com/apache/ignite/blob/master/modules/ > >> > > core/src/main/java/org/apache/ignite/internal/processors/ > >> > > cache/persistence/wal/FileWriteAheadLogManager.java#L253 > >> > > > [3] > >> > > > > >> > > > https://github.com/apache/ignite/blob/master/modules/ > >> > > core/src/main/java/org/apache/ignite/internal/processors/ > >> > > cache/persistence/wal/FileWriteAheadLogManager.java#L340 > >> > > > -- > >> > > > -- > >> > > > Maxim Muzafarov > >> > > > > >> > > > >> > > >> > > >> > > >> > -- > >> > Best regards, > >> > Ivan Pavlukhin > >> > > >> > > > > > > > > -- > > Best regards, > > Ivan Pavlukhin > > > > > > -- > Best regards, > Ivan Pavlukhin >
Re: Class field ThreadLocal. Why not static?
Alexey, Ivan, Agree. Keeping strong references to the Thread object is the source of memory leak with ThreadLocals variables and the values that it stores. ThreadLocalMap is bound to the Thread lifespan [1], so I think when we are using everything right all will be GC'ed correctly. Is this memory leaks with ThreadLocal's you mean, Alexey? If not, please, share your example. Also, agree that these usages should be bound to the component lifespan. But for `FileWriteAheadLogManager` I think this variable used not semantically right. I've dumped all threads (total ~49 threads) that are using `lastWalPtr` in `FileWriteAheadLogManager`. For instance, * exchange-worker-#40%wal.IgniteWalRecoveryTest0% * sys-#148%wal.IgniteWalRecoveryTest1% * db-checkpoint-thread-#129%wal.IgniteWalRecoveryTest2% Suppose everything would be OK here for `static` and `non-static` case of ThreadLocal. [1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/Thread.java#l760 On Tue, 11 Sep 2018 at 13:05 Павлухин Иван wrote: > Dmitriy, > > Could you point to some piece of code implementing described pattern? > > 2018-09-11 13:02 GMT+03:00 Павлухин Иван : > > > Alex, > > > > ThreadLocal subclass is used in IgniteH2Indexing for simple access to H2 > > Connection from current thread. Such subclass has a capability to create > > connection if one does not exist, so obtaining connection is merely > > ThreadLocal.get. Also there are scheduled routines to cleanup connections > > and associated with them statement cache after some expiration time. For > > that reason Map is maintained. As query can > > run on user thread we need to cleanup mentioned map to avoid a leak when > > Thread is terminated. So we need to check thread status in cleanup > routines > > and remove entries for terminated Threads. And historically there was no > > cleanup for terminated threads and leak was possible. And also great care > > must be taken in order to avoid cyclic reference between ThreadLocal > > instance and a stored value. Which easily could occur if the stored value > > is covered by multiple layers of abstraction. > > > > And I am describing some historical state. Now machinery in > IgniteH2Indexing > > is even more complex (I hope we will have a chance to improve it). > > > > 2018-09-11 11:00 GMT+03:00 Alexey Goncharuk >: > > > >> Ivan, > >> > >> Can you elaborate on the issue with the thread local cleanup you've > faced? > >> > >> вт, 11 сент. 2018 г. в 9:13, Павлухин Иван : > >> > >> > Guys, > >> > > >> > As we know ThreadLocal is an instrument which should be used with > great > >> > care. And I recently faced with problems related to proper cleanup of > >> > ThreadLocal which is not needed anymore. In my opinion the best thing > >> (in > >> > ideal world) is to get rid of ThreadLocal where possible, but I guess > >> that > >> > it is quite hard (in real world). > >> > > >> > Also, a question comes to my mind. As ThreadLocal is so common in our > >> code, > >> > could you suggest some guidance or code fragments which address proper > >> > ThreadLocal > >> > lifecycle control and especially cleanup? > >> > > >> > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk < > alexey.goncha...@gmail.com > >> >: > >> > > >> > > Maxim, > >> > > > >> > > Ignite supports starting multiple instances of Ignite in the same > VM, > >> so > >> > > having static thread locals for the fields you mentioned does not > >> work. > >> > > > >> > > Generally, I think thread-local should be bound to the lifespan of > the > >> > > component it describes. Static thread-locals are hard to clean-up > and > >> > they > >> > > often lead to leaks, so I would rather changed existing static > >> > > thread-locals to be non-static. > >> > > > >> > > --AG > >> > > > >> > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov : > >> > > > >> > > > Igniters, > >> > > > > >> > > > According to javadoc [1] class ThreadLocal: > >> > > > `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).` > >> > > > > >> > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per > >> class > >> > > > instance`. What the real cases of using non-static ThreadLocal > class > >> > > fields > >> > > > in Ignite code project? When we need it? > >> > > > > >> > > > In Ignite code project I've found ThreadLocal usage as: > >> > > > - non-static - 67 > >> > > > - static - 68 > >> > > > > >> > > > Back to my example, I've checked FileWriteAheadLogManager. It has: > >> > > > 1) private final ThreadLocal interrupted [2] > >> > > > 2) private final ThreadLocal lastWALPtr [3] > >> > > > I think both of these fields should be set and used as `static`. > Can > >> > > anyone > >> > > > confirm it? > >> > > > > >> > > > > >> > > > [1] > >> > https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html > >> > > > [2] > >> > > > > >> > > > https://githu
Re: Class field ThreadLocal. Why not static?
Dmitriy, Could you point to some piece of code implementing described pattern? 2018-09-11 13:02 GMT+03:00 Павлухин Иван : > Alex, > > ThreadLocal subclass is used in IgniteH2Indexing for simple access to H2 > Connection from current thread. Such subclass has a capability to create > connection if one does not exist, so obtaining connection is merely > ThreadLocal.get. Also there are scheduled routines to cleanup connections > and associated with them statement cache after some expiration time. For > that reason Map is maintained. As query can > run on user thread we need to cleanup mentioned map to avoid a leak when > Thread is terminated. So we need to check thread status in cleanup routines > and remove entries for terminated Threads. And historically there was no > cleanup for terminated threads and leak was possible. And also great care > must be taken in order to avoid cyclic reference between ThreadLocal > instance and a stored value. Which easily could occur if the stored value > is covered by multiple layers of abstraction. > > And I am describing some historical state. Now machinery in IgniteH2Indexing > is even more complex (I hope we will have a chance to improve it). > > 2018-09-11 11:00 GMT+03:00 Alexey Goncharuk : > >> Ivan, >> >> Can you elaborate on the issue with the thread local cleanup you've faced? >> >> вт, 11 сент. 2018 г. в 9:13, Павлухин Иван : >> >> > Guys, >> > >> > As we know ThreadLocal is an instrument which should be used with great >> > care. And I recently faced with problems related to proper cleanup of >> > ThreadLocal which is not needed anymore. In my opinion the best thing >> (in >> > ideal world) is to get rid of ThreadLocal where possible, but I guess >> that >> > it is quite hard (in real world). >> > >> > Also, a question comes to my mind. As ThreadLocal is so common in our >> code, >> > could you suggest some guidance or code fragments which address proper >> > ThreadLocal >> > lifecycle control and especially cleanup? >> > >> > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk > >: >> > >> > > Maxim, >> > > >> > > Ignite supports starting multiple instances of Ignite in the same VM, >> so >> > > having static thread locals for the fields you mentioned does not >> work. >> > > >> > > Generally, I think thread-local should be bound to the lifespan of the >> > > component it describes. Static thread-locals are hard to clean-up and >> > they >> > > often lead to leaks, so I would rather changed existing static >> > > thread-locals to be non-static. >> > > >> > > --AG >> > > >> > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov : >> > > >> > > > Igniters, >> > > > >> > > > According to javadoc [1] class ThreadLocal: >> > > > `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).` >> > > > >> > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per >> class >> > > > instance`. What the real cases of using non-static ThreadLocal class >> > > fields >> > > > in Ignite code project? When we need it? >> > > > >> > > > In Ignite code project I've found ThreadLocal usage as: >> > > > - non-static - 67 >> > > > - static - 68 >> > > > >> > > > Back to my example, I've checked FileWriteAheadLogManager. It has: >> > > > 1) private final ThreadLocal interrupted [2] >> > > > 2) private final ThreadLocal lastWALPtr [3] >> > > > I think both of these fields should be set and used as `static`. Can >> > > anyone >> > > > confirm it? >> > > > >> > > > >> > > > [1] >> > https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html >> > > > [2] >> > > > >> > > > https://github.com/apache/ignite/blob/master/modules/ >> > > core/src/main/java/org/apache/ignite/internal/processors/ >> > > cache/persistence/wal/FileWriteAheadLogManager.java#L253 >> > > > [3] >> > > > >> > > > https://github.com/apache/ignite/blob/master/modules/ >> > > core/src/main/java/org/apache/ignite/internal/processors/ >> > > cache/persistence/wal/FileWriteAheadLogManager.java#L340 >> > > > -- >> > > > -- >> > > > Maxim Muzafarov >> > > > >> > > >> > >> > >> > >> > -- >> > Best regards, >> > Ivan Pavlukhin >> > >> > > > > -- > Best regards, > Ivan Pavlukhin > -- Best regards, Ivan Pavlukhin
Re: Class field ThreadLocal. Why not static?
Alex, ThreadLocal subclass is used in IgniteH2Indexing for simple access to H2 Connection from current thread. Such subclass has a capability to create connection if one does not exist, so obtaining connection is merely ThreadLocal.get. Also there are scheduled routines to cleanup connections and associated with them statement cache after some expiration time. For that reason Map is maintained. As query can run on user thread we need to cleanup mentioned map to avoid a leak when Thread is terminated. So we need to check thread status in cleanup routines and remove entries for terminated Threads. And historically there was no cleanup for terminated threads and leak was possible. And also great care must be taken in order to avoid cyclic reference between ThreadLocal instance and a stored value. Which easily could occur if the stored value is covered by multiple layers of abstraction. And I am describing some historical state. Now machinery in IgniteH2Indexing is even more complex (I hope we will have a chance to improve it). 2018-09-11 11:00 GMT+03:00 Alexey Goncharuk : > Ivan, > > Can you elaborate on the issue with the thread local cleanup you've faced? > > вт, 11 сент. 2018 г. в 9:13, Павлухин Иван : > > > Guys, > > > > As we know ThreadLocal is an instrument which should be used with great > > care. And I recently faced with problems related to proper cleanup of > > ThreadLocal which is not needed anymore. In my opinion the best thing (in > > ideal world) is to get rid of ThreadLocal where possible, but I guess > that > > it is quite hard (in real world). > > > > Also, a question comes to my mind. As ThreadLocal is so common in our > code, > > could you suggest some guidance or code fragments which address proper > > ThreadLocal > > lifecycle control and especially cleanup? > > > > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk >: > > > > > Maxim, > > > > > > Ignite supports starting multiple instances of Ignite in the same VM, > so > > > having static thread locals for the fields you mentioned does not work. > > > > > > Generally, I think thread-local should be bound to the lifespan of the > > > component it describes. Static thread-locals are hard to clean-up and > > they > > > often lead to leaks, so I would rather changed existing static > > > thread-locals to be non-static. > > > > > > --AG > > > > > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov : > > > > > > > Igniters, > > > > > > > > According to javadoc [1] class ThreadLocal: > > > > `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).` > > > > > > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per > class > > > > instance`. What the real cases of using non-static ThreadLocal class > > > fields > > > > in Ignite code project? When we need it? > > > > > > > > In Ignite code project I've found ThreadLocal usage as: > > > > - non-static - 67 > > > > - static - 68 > > > > > > > > Back to my example, I've checked FileWriteAheadLogManager. It has: > > > > 1) private final ThreadLocal interrupted [2] > > > > 2) private final ThreadLocal lastWALPtr [3] > > > > I think both of these fields should be set and used as `static`. Can > > > anyone > > > > confirm it? > > > > > > > > > > > > [1] > > https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html > > > > [2] > > > > > > > > https://github.com/apache/ignite/blob/master/modules/ > > > core/src/main/java/org/apache/ignite/internal/processors/ > > > cache/persistence/wal/FileWriteAheadLogManager.java#L253 > > > > [3] > > > > > > > > https://github.com/apache/ignite/blob/master/modules/ > > > core/src/main/java/org/apache/ignite/internal/processors/ > > > cache/persistence/wal/FileWriteAheadLogManager.java#L340 > > > > -- > > > > -- > > > > Maxim Muzafarov > > > > > > > > > > > > > > > -- > > Best regards, > > Ivan Pavlukhin > > > -- Best regards, Ivan Pavlukhin
Re: Class field ThreadLocal. Why not static?
Hi Ivan, I can imagine cases with temporary native or heap byte buffers in thread locals used for IO operations. These buffers are often Thread Local and I find it to be a perfectly ok case. вт, 11 сент. 2018 г. в 11:00, Alexey Goncharuk : > Ivan, > > Can you elaborate on the issue with the thread local cleanup you've faced? > > вт, 11 сент. 2018 г. в 9:13, Павлухин Иван : > > > Guys, > > > > As we know ThreadLocal is an instrument which should be used with great > > care. And I recently faced with problems related to proper cleanup of > > ThreadLocal which is not needed anymore. In my opinion the best thing (in > > ideal world) is to get rid of ThreadLocal where possible, but I guess > that > > it is quite hard (in real world). > > > > Also, a question comes to my mind. As ThreadLocal is so common in our > code, > > could you suggest some guidance or code fragments which address proper > > ThreadLocal > > lifecycle control and especially cleanup? > > > > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk >: > > > > > Maxim, > > > > > > Ignite supports starting multiple instances of Ignite in the same VM, > so > > > having static thread locals for the fields you mentioned does not work. > > > > > > Generally, I think thread-local should be bound to the lifespan of the > > > component it describes. Static thread-locals are hard to clean-up and > > they > > > often lead to leaks, so I would rather changed existing static > > > thread-locals to be non-static. > > > > > > --AG > > > > > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov : > > > > > > > Igniters, > > > > > > > > According to javadoc [1] class ThreadLocal: > > > > `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).` > > > > > > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per > class > > > > instance`. What the real cases of using non-static ThreadLocal class > > > fields > > > > in Ignite code project? When we need it? > > > > > > > > In Ignite code project I've found ThreadLocal usage as: > > > > - non-static - 67 > > > > - static - 68 > > > > > > > > Back to my example, I've checked FileWriteAheadLogManager. It has: > > > > 1) private final ThreadLocal interrupted [2] > > > > 2) private final ThreadLocal lastWALPtr [3] > > > > I think both of these fields should be set and used as `static`. Can > > > anyone > > > > confirm it? > > > > > > > > > > > > [1] > > https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html > > > > [2] > > > > > > > > https://github.com/apache/ignite/blob/master/modules/ > > > core/src/main/java/org/apache/ignite/internal/processors/ > > > cache/persistence/wal/FileWriteAheadLogManager.java#L253 > > > > [3] > > > > > > > > https://github.com/apache/ignite/blob/master/modules/ > > > core/src/main/java/org/apache/ignite/internal/processors/ > > > cache/persistence/wal/FileWriteAheadLogManager.java#L340 > > > > -- > > > > -- > > > > Maxim Muzafarov > > > > > > > > > > > > > > > -- > > Best regards, > > Ivan Pavlukhin > > >
Re: Class field ThreadLocal. Why not static?
Ivan, Can you elaborate on the issue with the thread local cleanup you've faced? вт, 11 сент. 2018 г. в 9:13, Павлухин Иван : > Guys, > > As we know ThreadLocal is an instrument which should be used with great > care. And I recently faced with problems related to proper cleanup of > ThreadLocal which is not needed anymore. In my opinion the best thing (in > ideal world) is to get rid of ThreadLocal where possible, but I guess that > it is quite hard (in real world). > > Also, a question comes to my mind. As ThreadLocal is so common in our code, > could you suggest some guidance or code fragments which address proper > ThreadLocal > lifecycle control and especially cleanup? > > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk : > > > Maxim, > > > > Ignite supports starting multiple instances of Ignite in the same VM, so > > having static thread locals for the fields you mentioned does not work. > > > > Generally, I think thread-local should be bound to the lifespan of the > > component it describes. Static thread-locals are hard to clean-up and > they > > often lead to leaks, so I would rather changed existing static > > thread-locals to be non-static. > > > > --AG > > > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov : > > > > > Igniters, > > > > > > According to javadoc [1] class ThreadLocal: > > > `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).` > > > > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per class > > > instance`. What the real cases of using non-static ThreadLocal class > > fields > > > in Ignite code project? When we need it? > > > > > > In Ignite code project I've found ThreadLocal usage as: > > > - non-static - 67 > > > - static - 68 > > > > > > Back to my example, I've checked FileWriteAheadLogManager. It has: > > > 1) private final ThreadLocal interrupted [2] > > > 2) private final ThreadLocal lastWALPtr [3] > > > I think both of these fields should be set and used as `static`. Can > > anyone > > > confirm it? > > > > > > > > > [1] > https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html > > > [2] > > > > > > https://github.com/apache/ignite/blob/master/modules/ > > core/src/main/java/org/apache/ignite/internal/processors/ > > cache/persistence/wal/FileWriteAheadLogManager.java#L253 > > > [3] > > > > > > https://github.com/apache/ignite/blob/master/modules/ > > core/src/main/java/org/apache/ignite/internal/processors/ > > cache/persistence/wal/FileWriteAheadLogManager.java#L340 > > > -- > > > -- > > > Maxim Muzafarov > > > > > > > > > -- > Best regards, > Ivan Pavlukhin >
Re: Class field ThreadLocal. Why not static?
Guys, As we know ThreadLocal is an instrument which should be used with great care. And I recently faced with problems related to proper cleanup of ThreadLocal which is not needed anymore. In my opinion the best thing (in ideal world) is to get rid of ThreadLocal where possible, but I guess that it is quite hard (in real world). Also, a question comes to my mind. As ThreadLocal is so common in our code, could you suggest some guidance or code fragments which address proper ThreadLocal lifecycle control and especially cleanup? 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk : > Maxim, > > Ignite supports starting multiple instances of Ignite in the same VM, so > having static thread locals for the fields you mentioned does not work. > > Generally, I think thread-local should be bound to the lifespan of the > component it describes. Static thread-locals are hard to clean-up and they > often lead to leaks, so I would rather changed existing static > thread-locals to be non-static. > > --AG > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov : > > > Igniters, > > > > According to javadoc [1] class ThreadLocal: > > `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).` > > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per class > > instance`. What the real cases of using non-static ThreadLocal class > fields > > in Ignite code project? When we need it? > > > > In Ignite code project I've found ThreadLocal usage as: > > - non-static - 67 > > - static - 68 > > > > Back to my example, I've checked FileWriteAheadLogManager. It has: > > 1) private final ThreadLocal interrupted [2] > > 2) private final ThreadLocal lastWALPtr [3] > > I think both of these fields should be set and used as `static`. Can > anyone > > confirm it? > > > > > > [1] https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html > > [2] > > > > https://github.com/apache/ignite/blob/master/modules/ > core/src/main/java/org/apache/ignite/internal/processors/ > cache/persistence/wal/FileWriteAheadLogManager.java#L253 > > [3] > > > > https://github.com/apache/ignite/blob/master/modules/ > core/src/main/java/org/apache/ignite/internal/processors/ > cache/persistence/wal/FileWriteAheadLogManager.java#L340 > > -- > > -- > > Maxim Muzafarov > > > -- Best regards, Ivan Pavlukhin
Re: Class field ThreadLocal. Why not static?
Maxim, Ignite supports starting multiple instances of Ignite in the same VM, so having static thread locals for the fields you mentioned does not work. Generally, I think thread-local should be bound to the lifespan of the component it describes. Static thread-locals are hard to clean-up and they often lead to leaks, so I would rather changed existing static thread-locals to be non-static. --AG пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov : > Igniters, > > According to javadoc [1] class ThreadLocal: > `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).` > > So, AFAIK non-static ThreadLocal usage means as `per thread - per class > instance`. What the real cases of using non-static ThreadLocal class fields > in Ignite code project? When we need it? > > In Ignite code project I've found ThreadLocal usage as: > - non-static - 67 > - static - 68 > > Back to my example, I've checked FileWriteAheadLogManager. It has: > 1) private final ThreadLocal interrupted [2] > 2) private final ThreadLocal lastWALPtr [3] > I think both of these fields should be set and used as `static`. Can anyone > confirm it? > > > [1] https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html > [2] > > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/FileWriteAheadLogManager.java#L253 > [3] > > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/FileWriteAheadLogManager.java#L340 > -- > -- > Maxim Muzafarov >
Class field ThreadLocal. Why not static?
Igniters, According to javadoc [1] class ThreadLocal: `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).` So, AFAIK non-static ThreadLocal usage means as `per thread - per class instance`. What the real cases of using non-static ThreadLocal class fields in Ignite code project? When we need it? In Ignite code project I've found ThreadLocal usage as: - non-static - 67 - static - 68 Back to my example, I've checked FileWriteAheadLogManager. It has: 1) private final ThreadLocal interrupted [2] 2) private final ThreadLocal lastWALPtr [3] I think both of these fields should be set and used as `static`. Can anyone confirm it? [1] https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html [2] https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/FileWriteAheadLogManager.java#L253 [3] https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/persistence/wal/FileWriteAheadLogManager.java#L340 -- -- Maxim Muzafarov
