When I look at NumberToNumberConverterFactory, I find no instance cache for NumberToNumber.

public <T extends Number> Converter<Number, T> getConverter(Class<T> targetType) {
    return new NumberToNumber<>(targetType);
}

Therefore a new NumberToNumber object is instantiated for every conversion.

I added the following test case in DefaultConversionServiceTests, debugged it, and it really does create a new instance every time.

    @Test
    void frequencyConvert() {
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
        assertThat(conversionService.convert("1.0", Float.class)).isEqualTo(Float.valueOf("1.0"));
    }

Should NumberToNumberConverterFactor and StringToEnumConverterFactory introduce caches to avoid frequently creating new objects?

Comment From: kostya05983

Hello @sbrannen, I think we can add some static caches, but I'm not sure. Maybe it must work in more clever way with another caches. I attached a pr with static caches.

Comment From: bclozel

Are those allocations an actual performance issue? @yilianhuaixiao do you have an actual application showing performance problems because of this, or is this issue a simple observation?

@kostya05983 Thanks for the PR! At this point I'm really wondering if those allocations are really a performance problem. We could measure the tradeoffs with some caching. Note that your PR seems to be using an unbounded cache likely to be fed by untrusted input (for example, HTTP requests bound to controller arguments). This opens the door for security problems (DoS with OOM issues) or performance problems if the Map lookup takes longer that the actual object instantiation.

Spring Framework does have a shared ConcurrentLruCache implementation, but we've seen in benchmarks that it's really only worth it (the lookup + concurrency overhead) if the value is expensive to create in the first place.

I don't think this is the case here.

Comment From: quaff

@bclozel I agree with you but I think the cache is bounded, since key is targetType.getName() not arbitrary input.

Comment From: sbrannen

Spring Framework does have a shared ConcurrentLruCache implementation, but we've seen in benchmarks that it's really only worth it (the lookup + concurrency overhead) if the value is expensive to create in the first place.

I don't think this is the case here.

@jhoeller and I discussed this and came to the same conclusion.

In light of that, I am closing this issue and the corresponding PR (#25715).

Please note however, that if someone encounters a specific performance problem with these classes and provides benchmarks to analyze it, we would be willing to revisit this topic.