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

VELTOOLS-198 #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

VELTOOLS-198 #12

wants to merge 1 commit into from

Conversation

odoral
Copy link

@odoral odoral commented Nov 8, 2022

No description provided.

@michael-o
Copy link
Member

Now I wonder whether a TL would really be better than living without the cache. How big is the impact not using a cache?

@arkanovicz
Copy link
Contributor

arkanovicz commented Nov 8, 2022

Thanks for identifying this issue.

On this page someone had a measurement of ~23ms for the format instantiation. This is a figure which is highly dependent on the measurement conditions but still, it means that in terms of number of cycles its order of magnitude is to be taken into consideration.

The thread-local paradigm is not optimal here since in a web context for instance, requests are rarely made by the same thread. I would rather use a pool, each thread taking a format instance from the pool and putting it back afterwards. Apache commons has plenty of ready-made pools for this use case.

@michael-o
Copy link
Member

@arkanovicz While Commons Pool is very nice I use if for heavy objects as well, how do you want to expose its configuration, lifecycle, etc. to the client? As of now, the tools are more or less stateless.

@odoral
Copy link
Author

odoral commented Nov 8, 2022

Now I wonder whether a TL would really be better than living without the cache. How big is the impact not using a cache?

To be honest, we found this issue in a migration from 2.0 to 3.1 so not sure about how much this cache can improve. In our case, we've custom tools where we initialise formatters and they are used during the tool scope so there is just one call to get them.

I just moved cache into TL to not change design.

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.

4 participants