Make Compiler cache to soft value reference to reduce metaspace oom

[rice668]

Description

This PR can reduces metaspace oom issues, make some compiler that use guava cache value from strong reference to soft reference without obvious performance regression under 18,110 production queries.

Fixes #15232

Additional context and related issues

Release notes

( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[findepi]

Make Compiler cache to soft value reference to reduce metaspace oom

Trino caches do not use soft references, to avoid "invalidation storm" when system would otherwise OOM.
Instead, the system needs to be configured so that it doesn't OOM at all.

Also, if we really want to change this direction -- first make sure we want, which I personally doubt -- and start using soft references for all the caches, the change should focus on ensuring that all new caches are soft. I.e. please think about how to automatically ensure that. Modernizer may be helpful here, or some other code analysis.

[findepi]

cc @dain @lukasz-stec

[lukasz-stec]

Trino caches do not use soft references, to avoid "invalidation storm" when system would otherwise OOM.

Also, there are no guarantees for when soft references are freed other than before OOM. This can cause performance to be very variable.

That said, I see how cache size based on the number of elements can cause issues since the elements have variable lengths.
Generated classes bytecode, which I think is kept in the meta space, can vary between a few hundred bytes to hundreds of kilobytes or more.
One way to mitigate it is to limit the cache by bytes used (e.g. by using com.google.common.cache.Weigher).

[findepi]

That said, I see how cache size based on the number of elements can cause issues since the elements have variable lengths.

good point

This is also visible in #15232 description where singled out entry is 1/3 in size of the average

One way to mitigate it is to limit the cache by bytes used (e.g. by using com.google.common.cache.Weigher).

let's do that -- unless we know that max entry size * max entry count is still acceptable cache occupancy.

cc @dain for function & expression execution
cc @martint for #14237

[rice668]

Thanks to @findepi @lukasz-stec for discussion and related ideas. Limit the cache by bytes used com.google.common.cache.Weigher seems to be a more acceptable way to solve this problem. I'll revisit this issue in the near future and give some feedback or an update on the PR.

[rice668]

If we use weigher which requires calculate the class size of each entry value in int weight(K key, V value) . It seems that we can not find a suitable way to get it in currently. After discuss with @lukasz-stec , one possible solution is to modify ClassGenerator in airlift that let it return the cached generated classes and its size, etc. In addition to this solution, do we have a better way to get the size of the class ?

[electrum]

@zhangminglei apologies for the delay in responding.

I think the easiest way is to modify DynamicClassLoader in https://github.com/airlift/bytecode to have a method getDefinedClassesBytecodeSize(). It could track the size in defineClass(). We then add a method in CompilerUtils:

public static long getBytecodeSize(Class<?> clazz)
{
    if (clazz.getClassLoader() instanceof DynamicClassLoader loader) {
        return loader.getDefinedClassesBytecodeSize();
    }
    throw new IllegalArgumentException("Invalid class loader [%s] for %s".formatted(clazz.getClassLoader(), clazz));
}

[findepi]

    if (clazz.getClassLoader() instanceof DynamicClassLoader loader) {
        return loader.getDefinedClassesBytecodeSize();

do we assume DynamicClassLoader loads one class only?
or all classes loaded by one loader should report same weight?

[mosabua]

👋 @zhangminglei - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

[github-actions]

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

[mosabua]

Is this still relevant @electrum @findepi @rice668 ?

[electrum]

We can close this, as the cache weigher approach discussed above is a better direction.