Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow caching resolvedType in AbstractTypedElementDefinition #16567

Open
Simon-Hostettler opened this issue Apr 17, 2024 · 2 comments
Open

Allow caching resolvedType in AbstractTypedElementDefinition #16567

Simon-Hostettler opened this issue Apr 17, 2024 · 2 comments

Comments

@Simon-Hostettler
Copy link

Simon-Hostettler commented Apr 17, 2024

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

@simschla
Copy link

@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.

@lukaseder
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants