feat(#4218): avoid executing the injection logic multiple times #5201
Conversation
…Context to avoid executing the injection logic multiple times
|
|
||
| Object value = _injectablesCache.get(valueId); | ||
|
|
||
| if (value == null) { | ||
| value = _injectableValues.findInjectableValue(this, valueId, forProperty, beanInstance, | ||
| optional); | ||
|
|
||
| if (value != null) { | ||
| _injectablesCache.put(valueId, value); | ||
| } | ||
| } | ||
|
|
||
| return value; |
There was a problem hiding this comment.
Would this make injectino logic to run exactly once? Or less than before
There was a problem hiding this comment.
Exactly once, as proven by this test, which doesn't fail anymore with the change above
There was a problem hiding this comment.
I don't think that is true: while locating the value only occurs just once, wouldn't actual injection occur multiple times still? While this results in same value being used, it still calls injection (pass via constructor, set field) more than once. Ideally I'd rather neither would happen more than once.
There was a problem hiding this comment.
Correct. The value is either found in cache or actually resolved, but the injection happens anyway.
| @@ -169,6 +176,7 @@ protected DeserializationContext(DeserializerFactory df, | |||
| cache = new DeserializerCache(); | |||
| } | |||
| _cache = cache; | |||
| _injectablesCache = new ConcurrentHashMap<>(); | |||
There was a problem hiding this comment.
Need not be thread-safe, single deserialization calls are never concurrent. So simple HashMap would do.
Single DeserializationContext is only ever used for single readValue() call.
Also: should not eagerly construct Map since this imposes cost for all usage, even when (like in most cases I'd guess) no injection is required.
So should instead lazily construct when needed.
All of this assuming we actually want caching. Not sold on that yet either.
|
I don't really like this approach: to me it is working around the issue, instead of solving it. I'd rather that actual injection duplication is avoided, not just lookup. |
|
closing since it's a workaround not addressing the root cause of the issue |
|
Thank you still @giulong ! |
|
Opened #5224 with a better implementation |
Given there are multiple entry points for injection (creator properties vs fields and setters), would a cache in
DeserializationContextbe good to avoid executing the injection logic multiple times? Is the tradeoff of having a bit more space consumption vs much lower (I guess) execution time performance worth it?Especially considering that every new feature added in the
@JacksonInjectcontext, without a cache would mean duplicating the logic in a slightly different way, since creator properties and fields injection are managed differently.