Skip to content

Conversation

@rongrong
Copy link
Contributor

== RELEASE NOTES ==

General Changes
* Improve built-in function resolution performance by caching function resolution results.

@rongrong rongrong force-pushed the cache-function-resolution branch from 436bcff to 7e731ca Compare January 17, 2020 22:59
Copy link

@highker highker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

transactionManager.registerFunctionNamespaceManager(BuiltInFunctionNamespaceManager.ID, builtInFunctionNamespaceManager);
this.functionCache = CacheBuilder.newBuilder()
.recordStats()
.maximumSize(1000)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have 500+ functions, with high chances of different parameterTypes resolving to the same function, the cache could easily exceeds 1k entries after a reasonable amount of queries are executable. What about giving it a more generous size, like 10k? Or even better, make else it configurable?

Copy link
Contributor Author

@rongrong rongrong Jan 21, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is not to cache everything. Caching the top use cases is enough. The major target is the operators that's used over and over again in query planning (e.g., operator$equal(varchar, varchar) is used for every partition in PickTableLayout. Shadow testing showed that depending on the workload, No.1 hit can be 1 to 3 orders of magnitude higher than No.5. The overhead of resolving one function that's actually used in query is not that much. It's not clear to me that the processing time saved would worth the memory spent for the majority of the functions.

@rongrong rongrong merged commit 683474a into prestodb:master Jan 21, 2020
@caithagoras caithagoras mentioned this pull request Feb 20, 2020
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants