Cache accumulator factory #11358

dain · 2022-03-08T02:07:40Z

Description

Cache generated aggregation accumulator classes. When accumulator factory was split from function management we lost the caching of part of the generated code.

Related issues, pull requests, and links

Fixes Class loader leak since 367 #11234

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# General
* Fix regression in aggregation performance caused by not reusing generated accumulator factories. ({issue}`issuenumber`)

findepi

"Add getUnchecked to NonEvictableCache"

I do think we shouldn't add this method.

findepi · 2022-03-08T09:17:03Z

lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableCache.java

+            // this can not happen because a supplier can not throw a checked exception
+            throw new RuntimeException("Unexpected checked exception from cache load", e);


can -> should

(actually "it can happen", if you do some things you shouldn't do)

findepi · 2022-03-08T09:19:15Z

lib/trino-collect/src/main/java/io/trino/collect/cache/NonEvictableCache.java

 /**
 * A {@link com.google.common.cache.Cache} that does not support eviction.
 */
 public interface NonEvictableCache<K, V>
        extends Cache<K, V>
 {
+    default V getUnchecked(K key, Supplier<V> loader)


In Guava, getUnchecked is available on a LoadingCache only (and only without a loader).

I would prefer to stay in line with Guava APIs, so that we don't need to undo these simplifications, eg when switching back to Guava (should they fix the issue), or refactoring the code.

Thus, while I introduced all the "EvictableCache" related class, I wouldn't yet want to embrace them as "an API".

If, however, we wanted to depart from Guava APIs, i would prefer we have a different signature

V <T extends Exception> get(K key, ThrowingSupplier<T> loader) throws T;

this would erase the (logically useless) distinction between get and getUnchecked, i.e. the caller wouldn't need to make the choice.

@findepi and I talked about this off line. The problem with an API like this is exceptions can cross threads when there are concurrent calls to a single element. The cache synchronizes and shares the result for all callers, which means the threads will shared the stacks from the one thread that actuallu executed the loader.

findepi · 2022-03-08T09:24:04Z

Was the leak only due to the fact we weren't caching things?
i.e. would the generated classes be eventually garbage-collected anyway?

guyco33 · 2022-03-08T10:48:51Z

i.e. would the generated classes be eventually garbage-collected anyway?

There are still other generated classes that are not garbage-collected like:
io.trino.$gen.CursorProcessor
io.trino.$gen.PageFilter
io.trino.$gen.PageProjectionWork

trino-test-372-3-g59c5ca1 (worker) root@$ 
jcmd trino VM.class_hierarchy  | egrep 'io[.]trino[.][$]gen[.].+[0-9]{8,8}[_][0-9]{6,6}[_][0-9]+[/]' | awk -F_ '{arr[$1]++}END{for (a in arr) print a, arr[a]}' | sort

|  |--io.trino.$gen.GroupedGenericLongState 3
|  |--io.trino.$gen.GroupedLongLongState 3
|  |--io.trino.$gen.GroupedLongState 12
|--io.trino.$gen.BlockPositionComparison 1
|--io.trino.$gen.BlockPositionEqual 1
|--io.trino.$gen.BlockPositionHashCode 1
|--io.trino.$gen.CursorProcessor 2540
|--io.trino.$gen.GenericLongStateFactory 3
|--io.trino.$gen.LongLongStateFactory 3
|--io.trino.$gen.LongLongStateSerializer 3
|--io.trino.$gen.LongStateFactory 12
|--io.trino.$gen.LongStateSerializer 12
|--io.trino.$gen.PageFilter 1975
|--io.trino.$gen.PageProjectionWork 4376
|--io.trino.$gen.SingleGenericLongState 3
|--io.trino.$gen.SingleLongLongState 3
|--io.trino.$gen.SingleLongState 12
|--io.trino.$gen.countAccumulator 12
|--io.trino.$gen.countGroupedAccumulator 12
|--io.trino.$gen.maxAccumulator 3
|--io.trino.$gen.maxGroupedAccumulator 3
|--io.trino.$gen.sumAccumulator 3
|--io.trino.$gen.sumGroupedAccumulator 3

sopel39 · 2022-03-08T10:55:56Z

There are still other generated classes that are not garbage-collected like:

io.trino.$gen.CursorProcessor
io.trino.$gen.PageFilter
io.trino.$gen.PageProjectionWork

these are actually cached. Do you say cache is disabled and they are not GCed anyway?

guyco33 · 2022-03-08T11:03:35Z

Do you say cache is disabled and they are not GCed anyway

JVM is up for more than 4 hours and I can still see generated classes from 06:58:11

trino-test-372-3-g59c5ca1 (worker) root@$ 
jcmd trino VM.uptime | tail -1 && jcmd trino VM.class_hierarchy | egrep 'io[.]trino[.][$]gen[.]CursorProcessor' | sort| more
15018.827 s
|--io.trino.$gen.CursorProcessor_20220308_065811_20/0x00005598c03a6140
|--io.trino.$gen.CursorProcessor_20220308_065812_25/0x00007ffa60106940
|--io.trino.$gen.CursorProcessor_20220308_065828_74/0x00007ffa3c10c620

guyco33 · 2022-03-08T11:20:52Z

Was the leak only due to the fact we weren't caching things?

I can approve that io.trino.$gen.*Accumulator_* is not leaking as before

sopel39 · 2022-03-08T12:49:56Z

JVM is up for more than 4 hours and I can still see generated classes from 06:58:11

PageFunctionCompiler cache is not time based, only size based

guyco33 · 2022-03-08T13:59:03Z

PageFunctionCompiler cache is not time based, only size based

Got it. And the default is 10,000 entries

guyco33 · 2022-03-08T14:13:16Z

It seems that we need also time based cache here

trino-test-372-3-g59c5ca1 (worker) root@$ 
jcmd trino VM.class_hierarchy  | egrep 'io[.]trino[.][$]gen[.].+[0-9]{8,8}[_][0-9]{6,6}[_][0-9]+[/]' | awk -F_ '{arr[$1]++}END{for (a in arr) if (arr[a] > 100) {print a, arr[a]}}' | sort

|--io.trino.$gen.CursorProcessor 5029
|--io.trino.$gen.PageFilter 4968
|--io.trino.$gen.PageProjectionWork 29948

guyco33 · 2022-03-10T06:54:26Z

After running a load test for almost 2 days, I approve that this fix did the job.
It solve the memory leak seen in 367+ and fixes the SQL performance degradation.

dain · 2022-03-10T23:44:02Z

JVM is up for more than 4 hours and I can still see generated classes from 06:58:11

PageFunctionCompiler cache is not time based, only size based

@sopel39 we should consider limiting it to an hour like the others

sopel39 · 2022-03-11T09:34:31Z

@sopel39 we should consider limiting it to an hour like the others

Makes sense. I don't think it would hurt. @guyco33 would you like to do so?

for "Add getUnchecked to NonEvictableCache" -- discussed the plan

findepi · 2022-03-11T11:35:45Z

Was the leak only due to the fact we weren't caching things? i.e. would the generated classes be eventually garbage-collected anyway?

@dain can you please explain this?

dain · 2022-03-11T16:58:32Z

Was the leak only due to the fact we weren't caching things? i.e. would the generated classes be eventually garbage-collected anyway?

I believe so. Based on @guyco33's testing I'm pretty confident.

martint · 2022-03-11T17:03:11Z

can you please explain this?

Yes, the "leak" is not truly a leak -- the VM will eventually collect those classes if they are no longer referenced. The performance issue is due to not reusing the generated classes, so the VM is in an eternal warming up state for aggregations.

guyco33 · 2022-03-13T08:08:02Z

the VM will eventually collect those classes if they are no longer referenced

but it seems that they retained since the reference never gone

The uncheckedCacheGet method uses a Supplier instead of a Callable and therefore can not throw a checked exception, so a checked is not thrown from uncheckedCacheGet. This simplifies most callers which do not use checked exceptions.

Remove LambdaProvider instances from AccumulatorFactory to simplify caching of the factories.

rice668 · 2022-12-06T09:32:23Z

the VM will eventually collect those classes if they are no longer referenced

but it seems that they retained since the reference never gone

Maybe you are interesting in this #15237 why reference never gone

cla-bot bot added the cla-signed label Mar 8, 2022

findepi previously requested changes Mar 8, 2022

View reviewed changes

guyco33 mentioned this pull request Mar 11, 2022

Expire PageFunction and CursorProcessor caches by time #11434

Closed

dain force-pushed the cache-accumulator-factory branch 3 times, most recently from 11d268c to 2c04ca5 Compare March 12, 2022 03:28

dain added 3 commits March 13, 2022 13:06

Add CacheUtils.uncheckedCacheGet helper

e966733

The uncheckedCacheGet method uses a Supplier instead of a Callable and therefore can not throw a checked exception, so a checked is not thrown from uncheckedCacheGet. This simplifies most callers which do not use checked exceptions.

Pass LambdaProviders to AccumulatorFactory instead of storing them

0f1455d

Remove LambdaProvider instances from AccumulatorFactory to simplify caching of the factories.

Cache generated AccumulatorFactory classes

081f153

dain force-pushed the cache-accumulator-factory branch from 2c04ca5 to 081f153 Compare March 13, 2022 20:19

github-actions bot added the tests:hive label Mar 13, 2022

martint approved these changes Mar 14, 2022

View reviewed changes

dain merged commit 2bfd417 into trinodb:master Mar 14, 2022

dain deleted the cache-accumulator-factory branch March 14, 2022 18:30

mosabua mentioned this pull request Mar 14, 2022

Add Trino 374 release notes #11417

Merged

github-actions bot added this to the 374 milestone Mar 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache accumulator factory #11358

Cache accumulator factory #11358

dain commented Mar 8, 2022

findepi left a comment

findepi Mar 8, 2022

findepi Mar 8, 2022

findepi Mar 8, 2022

dain Mar 11, 2022

findepi commented Mar 8, 2022

guyco33 commented Mar 8, 2022 •

edited

Loading

sopel39 commented Mar 8, 2022

guyco33 commented Mar 8, 2022 •

edited

Loading

guyco33 commented Mar 8, 2022

sopel39 commented Mar 8, 2022

guyco33 commented Mar 8, 2022 •

edited

Loading

guyco33 commented Mar 8, 2022 •

edited

Loading

guyco33 commented Mar 10, 2022

dain commented Mar 10, 2022

sopel39 commented Mar 11, 2022

findepi commented Mar 11, 2022

dain commented Mar 11, 2022

martint commented Mar 11, 2022

guyco33 commented Mar 13, 2022

rice668 commented Dec 6, 2022 •

edited

Loading

		// this can not happen because a supplier can not throw a checked exception
		throw new RuntimeException("Unexpected checked exception from cache load", e);

Cache accumulator factory #11358

Cache accumulator factory #11358

Conversation

dain commented Mar 8, 2022

Description

Related issues, pull requests, and links

Documentation

Release notes

findepi left a comment

Choose a reason for hiding this comment

findepi Mar 8, 2022

Choose a reason for hiding this comment

findepi Mar 8, 2022

Choose a reason for hiding this comment

findepi Mar 8, 2022

Choose a reason for hiding this comment

dain Mar 11, 2022

Choose a reason for hiding this comment

findepi commented Mar 8, 2022

guyco33 commented Mar 8, 2022 • edited Loading

sopel39 commented Mar 8, 2022

guyco33 commented Mar 8, 2022 • edited Loading

guyco33 commented Mar 8, 2022

sopel39 commented Mar 8, 2022

guyco33 commented Mar 8, 2022 • edited Loading

guyco33 commented Mar 8, 2022 • edited Loading

guyco33 commented Mar 10, 2022

dain commented Mar 10, 2022

sopel39 commented Mar 11, 2022

findepi commented Mar 11, 2022

dain commented Mar 11, 2022

martint commented Mar 11, 2022

guyco33 commented Mar 13, 2022

rice668 commented Dec 6, 2022 • edited Loading

guyco33 commented Mar 8, 2022 •

edited

Loading

guyco33 commented Mar 8, 2022 •

edited

Loading

guyco33 commented Mar 8, 2022 •

edited

Loading

guyco33 commented Mar 8, 2022 •

edited

Loading

rice668 commented Dec 6, 2022 •

edited

Loading