yes, they need to be threadsafe if you cache them. numberformat itself is also not threadsafe. another reason i can think of why we clone is that a code that gets a bigdecimalconverter via getconverter() can execute setters on it and modify it- which will affect the converter globally. im not really sure how important the latter is since you would have to downcast the interface, but still. perhaps some sort of instance cache is in order.
-igor On Mon, Feb 9, 2009 at 1:26 AM, Martin Makundi <[email protected]> wrote: >> why dont you create your own converter for now and see how that >> improves things. we are talking about something that took 1.4 seconds >> out of at least a 100+ second test... > > Ok, I am refactoring my code right now. It is not a 100 second test.. > the profiler is running while I am analyzing it so the (idle) server > thread is not related. The test duration can be observed from the > WicketServlet.service -call, which is about 24 seconds. I am concerned > about BigDecimalConverter<init> and > BigDecimalConverter.getNumberFormat because they are the only > "intrinsic" operations that show up on the hotspot list. Everything > else that show's up is more or less 'business logic'. I would prefer > the fundamental intrinsic stuff from the framework not to show up in > the test :) > > ::: Results after creating my own ParametrizedConverterLocator: > > - the profiler does not show any more of those BigDecimalConverters. > Why? Because I cache them in ParametrizedConverterLocator using a > WeakHashMap. Also the NumberFormat instantiations have disappeared > because it is initializede only once and all the later invocations use > the cloned copy. It appears that there is really no need to hassle > with a threadLocal NumberFormat. > > I wonder if I should make the ParametrizedConverterLocator converter > map thread-safe? The wicket's ConverterLocator.classToConverter > appears non thread-safe, from what I can see. Can that become a > problem? > > ** > Martin > >> On Sun, Feb 8, 2009 at 11:31 PM, Martin Makundi >> <[email protected]> wrote: >>>> im not sure how necessary the threadlocal is. yes, it took up 6mb of >>>> memory, but those instances are not being held on to, so if you run GC >>>> all that memory will be reclaimed. >>> >>> I am more concerned with the cpu hog than the memory hog associated >>> with the converter. >>> >>> ** >>> Martin >>> >>>> >>>> On Sun, Feb 8, 2009 at 8:55 PM, Martin Makundi >>>> <[email protected]> wrote: >>>>> Cool. What is your take on using a ThreaLocal instead of always >>>>> cloning return (NumberFormat)numberFormat.clone(); in >>>>> org.apache.wicket.util.convert.converters.AbstractDecimalConverter#getNumberFormat(java.util.Locale) >>>>> >>>>> ** >>>>> Martin >>>>> >>>>> 2009/2/9 Igor Vaynberg <[email protected]>: >>>>>> On Sun, Feb 8, 2009 at 7:46 PM, Martin Makundi >>>>>> <[email protected]> wrote: >>>>>>> Another thing that came into my mind is, that there is >>>>>>> newNumberFormat(Locale locale) -method in AbstractDecimalConverter, >>>>>>> but the method is never used [instead, there is a direct "numberFormat >>>>>>> = NumberFormat.getInstance(locale);" -invocation in >>>>>>> getNumberFormat(Locale locale)]. >>>>>> >>>>>> fixed >>>>>> >>>>>> -igor >>>>>> >>>>>> --------------------------------------------------------------------- >>>>>> To unsubscribe, e-mail: [email protected] >>>>>> For additional commands, e-mail: [email protected] >>>>>> >>>>>> >>>>> >>>>> --------------------------------------------------------------------- >>>>> To unsubscribe, e-mail: [email protected] >>>>> For additional commands, e-mail: [email protected] >>>>> >>>>> >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe, e-mail: [email protected] >>>> For additional commands, e-mail: [email protected] >>>> >>>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe, e-mail: [email protected] >>> For additional commands, e-mail: [email protected] >>> >>> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: [email protected] >> For additional commands, e-mail: [email protected] >> >> > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
