search

Chrono24 / stripes

Stripes is a Java framework with the goal of making Servlet/JSP based web development in Java as easy, intuitive and straight-forward as it should be. It's stripey and it doesn't suck.
8 stars 5 forks source link

Cache NumberFormat.getInstance #24

Open keiki85 opened 2 years ago

keiki85 commented 2 years ago

NumberFormat can be reused and quite often the same can be used. So we should cache it per thread for performance reason.

NumberFormat.getInstance is quite expensive if executed thousands of times.

udittmer commented 2 years ago

You should optimize its usage that way only if you need to; have you run tests that show this would make a difference? See this discussion on StackOverflow, where the consensus seemed to be that most likely it would not make a difference.

A complication in Stripes is that not all Numberformat instances are alike: locale, grouping, number of digits and even the pattern may change dynamically - so you would need a fairly elaborate cache of different NumberFormat instances. Take a look at the net.sourceforge.stripes.format.NumberFormatter class to see how this works.

keiki85 commented 2 years ago

In my case it made a difference of a few seconds when loading a page, where this is used heavily.

For me it looks like in this case the benefit of caching is bigger than recreating it every time. Also the size of a NumberFormat isn't that huge that memory would bloat.

Also here I'm talking about NumberFormat.getInstance(locale), which is doing more stuff than just new DecimalFormat refered to in stackoverflow.

udittmer commented 2 years ago

Are the cached instances safe to reuse? The setGroupingUsed, setMaximumFractionDigits and setMinimumFractionDigits methods are only called under certain circumstances, not all circumstances. For example, if you use it with a formatPattern of "plain", grouping is switched off, and if the next call has a pattern of "integer", it's not turned back on. Or maybe I'm under-caffeinated and not making sense :-)

keiki85 commented 2 years ago

Thanks for raising this.

My current caching implementation is grouped by type (number, %, currency) and locale. So I would assume that those are already set correctly.

See https://github.com/Chrono24/stripes/pull/25

udittmer commented 2 years ago

No, they're not. It's sufficient to consider just numbers, and not even locales. The problem is that the state of the cached object is changed with each invocation, but the code relies on the defaults. The attached code demonstrates how reusing a NumberFormat instance without calling all the setup methods (as the NumberFormatter class does) results in different results, depending on the order in which it is called with different formatPattern parameters.

The settings controlled by the setGroupingUsed, setMaximumFractionDigits, setMinimumFractionDigits and applyPattern methods now all need to be re-initialized each time init() is called.

NumberFormatTest.txt

keiki85 commented 2 years ago

I'm getting now where you are coming from and where the issue lies. I was too much focused on the getInstance part that I overlooked the part where the type format is configured in detail.

So in my next spare time I will have a look at it and also check if it still makes sense to cache it.

Thank you and cheers.