jOOQ icon indicating copy to clipboard operation
jOOQ copied to clipboard

Allow caching resolvedType in AbstractTypedElementDefinition

Open Simon-Hostettler opened this issue 2 years ago • 2 comments

Use case

For Issue #15553 the AbstractTypedElementDefinition.resolvedType field and its use in the getType(JavaTypeResolver resolver) method were removed. This causes the AbstractDatabase.getConfiguredForcedType(Definition, DataTypeDefinition) method to be called repeatedly instead of once. Our task includes ~400 forced types and ~450 tables across 9 schemas, resulting in this call being rather expensive at up to 10 seconds per invocation. With these changes, the task runtime went from 23 to 330 seconds. It would be great to be able to toggle this caching behaviour to reduce compile times.

Possible solution

A possible solution would be to add a configuration option that would re-enable this caching behaviour. Since it caused some problems, this should probably be false per default.

Another solution would be to remove the final keyword from AbstractDatabase.getConfiguredForcedType, so we could extend PostgresDatabase with a method that implements caching.

Possible workarounds

To test that this was indeed the cause, I profiled the task once with default jOOQ 3.19.7 and once by patching the jar and adding back the following lines in AbstractTypedElementDefinition:

private transient DataTypeDefinition resolvedType;

...

@Override
    public DataTypeDefinition getType(JavaTypeResolver resolver) {
        if (resolvedType == null)
            resolvedType = mapDefinedType(container, this, definedType, resolver);

        return resolvedType;
    }

The result of the first profiling run: Screenshot 2024-04-17 at 16 03 45

and the second: Screenshot 2024-04-17 at 16 04 55

Of course this isn't really a feasible workaround. Maybe overriding getConfiguredForcedType in a custom Database and implementing a custom cache would be an option?

jOOQ Version

jOOQ Open Source Edition 3.19.7

Database product and version

PostgreSQL 16.2 (Debian 16.2-1.pgdg110+2) on ARM64

Java Version

No response

JDBC / R2DBC driver name and version (include name if unofficial driver)

No response

Simon-Hostettler avatar Apr 17 '24 14:04 Simon-Hostettler

@lukaseder As you can see, the size and extent of our database contradicts the original assumption in #15553:

~The cache probably doesn't speed up things drastically, so it's not too bad if we remove it.

We suffer a severe performance decrease, the code generation takes 20x as long as with the cache, what used to be seconds now takes minutes.

As @Simon-Hostettler brought up: Is there any way we can make the usage of the (previous) cache configurable - or would you be willing to open the API in a way, that we can provide such a cache ourselves? Currently, almost everything is either private or static or both, so extending/patching the generator code to allow using such a cache is pretty ugly at best, or impossible.

simschla avatar Apr 18 '24 05:04 simschla

Thanks for the comments, folks. I'll look into this soon. If there's indeed such a change in performance, I'd say this is a regression.

lukaseder avatar Apr 18 '24 17:04 lukaseder