Allow to automatically trim the PoolThreadCache in a timely interval #8941

normanmaurer · 2019-03-15T08:46:02Z

Motivation:

PooledByteBufAllocator uses a PoolThreadCache per Thread that allocates / deallocates to minimize the performance overhead. This PoolThreadCache is trimmed after X allocations to free up buffers that are not allocated for a long time. This works out quite well when the app continues to allocate but fails if the app stops to allocate frequently (for whatever reason) and so a lot of memory is wasted and not given back to the arena / freed.

Modifications:

Add a ThreadExecutorMap that offers multiple methods that wrap Runnable / ThreadFactory / Executor and allow to call ThreadExecutorMap.currentEventExecutor() to get the current executing EventExecutor for the calling Thread.
Use these methods in the constructors of our EventExecutor implementations (which also covers the EventLoop implementations)
Add io.netty.allocator.cacheTrimIntervalMillis system property which can be used to specify a fixed rate / interval on which we should try to trim the PoolThreadCache for a EventExecutor that allocates.
Add PooledByteBufAllocator.trimCurrentThreadCache() to allow the user to trim the cache of the calling thread manually.
Add testcases
Introduce FastThreadLocal.getIfExists()

Result:

Allow to better / more frequently trim PoolThreadCache and so give back memory to the area / system.

johnou · 2019-03-15T08:53:52Z

buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java

+                        executor.scheduleAtFixedRate(new Runnable() {
+                            @Override
+                            public void run() {
+                                cache.trim();


try catch Exception to avoid suppressing subsequent executions?

@johnou this should never throw, if it does there is something seriously wrong

@normanmaurer but unless the app developer has hooked into java.lang.Thread#setDefaultUncaughtExceptionHandler they are not going to know and the exception will be swallowed..

common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java

johnou

left a few suggestions

eolivelli · 2019-03-15T09:58:16Z

awesome @normanmaurer
I will take a deeper look as soon as possible

buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java

eolivelli · 2019-03-15T11:32:55Z

common/src/test/java/io/netty/util/internal/ThreadExecutorMapTest.java

+    }
+
+    @Test
+    public void testDecorateTheadFactory() throws InterruptedException {


nit: typo, missing 'r'

eolivelli

Very useful!
LGTM

there is a little typo in a test

martinfurmanski

Looks good to me! Thanks for the quick turnaround.

njhill

@normanmaurer nice change! One comment inline.

Also not really part of this change but was wondering if there's any reason why GlobalEventExecutor doesn't implement OrderedEventExecutor?

njhill · 2019-03-15T18:48:31Z

common/src/main/java/io/netty/util/concurrent/UnorderedThreadPoolEventExecutor.java

@@ -59,6 +60,7 @@ public UnorderedThreadPoolEventExecutor(int corePoolSize) {
     */
    public UnorderedThreadPoolEventExecutor(int corePoolSize, ThreadFactory threadFactory) {
        super(corePoolSize, threadFactory);
+        this.setThreadFactory(ThreadExecutorMap.apply(getThreadFactory(), this));


Is this is safe? I think the scheduled repeating trimming task could be run on an arbitrary thread, i.e. not necessarily the one from which it was scheduled that owns the referenced PoolThreadCache instance.

Maybe for this one the scheduled task could just call PoolThreadCache.trimCurrentThreadCache()?

you are right... let me fix it

@njhill also with this in mind I wonder if we should maybe restrict all of this to OrderedEventExecutor

@njhill done that ....

Thanks @normanmaurer generally LGTM, but I'm not sure about the OrderedEventExecutor check since my understanding is that although SingleThreadEventExecutor and GlobalEventExecutor are ok, that interface doesn't necessarily imply same-thread semantics, only serialized tasks. So having the check there might be misleading... it's necessary but not sufficient. Maybe we could have a static scheduledTasksRunOnSameThread(EventExecutor) helper method which for now returns true only in the case of STEE and GEE?

(but I still like the change to make GEE implement OEE!)

@njhill maybe we should just only enable it for SingleThreadEventExecutor for now. Let me do this.

@normanmaurer that's probably fine but technically there's still a chance that someone has subclassed it with different scheduleAtFixedRate behaviour that breaks the assertion, since the method is non-final (though this wouldn't really make much sense to do).

@njhill true.. I guess we could also just return directly if the Thread is not the same we expect. WDYT ?

@njhill alternatively I just noticed we could just call:

PooledByteBufAllocator.this.trimCurrentThreadCache();

This should be safe in any case.

@normanmaurer yes, that would be safe for any of the cases (worst case it would be a repeating no-op)... that's actually what I was meaning to suggest above but obviously got the detail wrong since that method is on the BBAllocator not PoolThreadCache!

trustin · 2019-03-18T10:42:03Z

buffer/src/main/java/io/netty/buffer/PooledByteBufAllocator.java

@@ -113,6 +118,9 @@
        DEFAULT_CACHE_TRIM_INTERVAL = SystemPropertyUtil.getInt(
                "io.netty.allocator.cacheTrimInterval", 8192);

+        DEFAULT_CACHE_TRIM_RATE = SystemPropertyUtil.getLong(
+                "io.netty.allocator.cacheTrimRateMs", 0);


How about io.netty.allocation.cacheTrimIntervalMillis? To avoid confusion, we could deprecate cacheTrimInterval in favor of cacheTrimIntervalCount. The current name rate is misleading because it's not really a rate.

@trustin I think I would like to still preserve the old one as well. Let me rename it tho.

common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java

trustin · 2019-03-18T10:44:00Z

common/src/main/java/io/netty/util/internal/ThreadExecutorMap.java

+public final class ThreadExecutorMap {
+
+    private static final FastThreadLocal<EventExecutor> mappings =
+            new FastThreadLocal<EventExecutor>();


nit: could be merged into a single line.

common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java

Motivation: PooledByteBufAllocator uses a PoolThreadCache per Thread that allocates / deallocates to minimize the performance overhead. This PoolThreadCache is trimmed after X allocations to free up buffers that are not allocated for a long time. This works out quite well when the app continues to allocate but fails if the app stops to allocate frequently (for whatever reason) and so a lot of memory is wasted and not given back to the arena / freed. Modifications: - Add a ThreadExecutorMap that offers multiple methods that wrap Runnable / ThreadFactory / Executor and allow to call ThreadExecutorMap.currentEventExecutor() to get the current executing EventExecutor for the calling Thread. - Use these methods in the constructors of our EventExecutor implementations (which also covers the EventLoop implementations) - Add io.netty.allocator.cacheTrimIntervalMillis system property which can be used to specify a fixed rate / interval on which we should try to trim the PoolThreadCache for a EventExecutor that allocates. - Add PooledByteBufAllocator.trimCurrentThreadCache() to allow the user to trim the cache of the calling thread manually. - Add testcases - Introduce FastThreadLocal.getIfExists() Result: Allow to better / more frequently trim PoolThreadCache and so give back memory to the area / system.

normanmaurer · 2019-03-22T08:04:54Z

Just rebased it.

…8941) Motivation: PooledByteBufAllocator uses a PoolThreadCache per Thread that allocates / deallocates to minimize the performance overhead. This PoolThreadCache is trimmed after X allocations to free up buffers that are not allocated for a long time. This works out quite well when the app continues to allocate but fails if the app stops to allocate frequently (for whatever reason) and so a lot of memory is wasted and not given back to the arena / freed. Modifications: - Add a ThreadExecutorMap that offers multiple methods that wrap Runnable / ThreadFactory / Executor and allow to call ThreadExecutorMap.currentEventExecutor() to get the current executing EventExecutor for the calling Thread. - Use these methods in the constructors of our EventExecutor implementations (which also covers the EventLoop implementations) - Add io.netty.allocator.cacheTrimIntervalMillis system property which can be used to specify a fixed rate / interval on which we should try to trim the PoolThreadCache for a EventExecutor that allocates. - Add PooledByteBufAllocator.trimCurrentThreadCache() to allow the user to trim the cache of the calling thread manually. - Add testcases - Introduce FastThreadLocal.getIfExists() Result: Allow to better / more frequently trim PoolThreadCache and so give back memory to the area / system.

normanmaurer requested review from ejona86, carl-mastrangelo and trustin March 15, 2019 08:46

normanmaurer added this to the 4.1.35.Final milestone Mar 15, 2019

normanmaurer self-assigned this Mar 15, 2019

normanmaurer added improvement feature labels Mar 15, 2019

johnou reviewed Mar 15, 2019

View reviewed changes

common/src/main/java/io/netty/util/concurrent/GlobalEventExecutor.java Show resolved Hide resolved

johnou suggested changes Mar 15, 2019

View reviewed changes

eolivelli reviewed Mar 15, 2019

View reviewed changes

eolivelli approved these changes Mar 15, 2019

View reviewed changes

normanmaurer force-pushed the thread_executor_map branch from adad8d0 to 880cf66 Compare March 15, 2019 15:49

martinfurmanski approved these changes Mar 15, 2019

View reviewed changes

njhill reviewed Mar 15, 2019

View reviewed changes

trustin reviewed Mar 18, 2019

View reviewed changes

normanmaurer force-pushed the thread_executor_map branch 2 times, most recently from 7852e46 to 8dead9a Compare March 19, 2019 11:05

normanmaurer force-pushed the thread_executor_map branch from 8dead9a to d78225d Compare March 22, 2019 08:04

trustin approved these changes Mar 22, 2019

View reviewed changes

normanmaurer merged commit c83904a into 4.1 Mar 22, 2019

normanmaurer deleted the thread_executor_map branch March 22, 2019 10:08

Allow to automatically trim the PoolThreadCache in a timely interval #8941

Allow to automatically trim the PoolThreadCache in a timely interval #8941

Conversation

normanmaurer commented Mar 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

johnou left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli commented Mar 15, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

martinfurmanski left a comment

Choose a reason for hiding this comment

Uh oh!

njhill left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

normanmaurer commented Mar 22, 2019

Uh oh!

normanmaurer commented Mar 15, 2019 •

edited

Loading