Reduce boxing once.
Comment From: sbrannen
Thanks for the PR.
Executing javap -c spring-context/build/classes/java/main/org/springframework/cache/interceptor/CacheAspectSupport\$CacheOperationContext.class reveals the following.
protected boolean isConditionPassing(java.lang.Object);
Code:
0: aload_0
1: getfield #18 // Field conditionPassing:Ljava/lang/Boolean;
4: ifnonnull 75
7: aload_0
8: getfield #1 // Field metadata:Lorg/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata;
11: invokestatic #13 // Method org/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata.access$300:(Lorg/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata;)Lorg/springframework/cache/interceptor/CacheOperation;
14: invokevirtual #19 // Method org/springframework/cache/interceptor/CacheOperation.getCondition:()Ljava/lang/String;
17: invokestatic #20 // Method org/springframework/util/StringUtils.hasText:(Ljava/lang/String;)Z
20: ifeq 67
23: aload_0
24: aload_1
25: invokespecial #21 // Method createEvaluationContext:(Ljava/lang/Object;)Lorg/springframework/expression/EvaluationContext;
28: astore_2
29: aload_0
30: aload_0
31: getfield #2 // Field this$0:Lorg/springframework/cache/interceptor/CacheAspectSupport;
34: invokestatic #22 // Method org/springframework/cache/interceptor/CacheAspectSupport.access$700:(Lorg/springframework/cache/interceptor/CacheAspectSupport;)Lorg/springframework/cache/interceptor/CacheOperationExpressionEvaluator;
37: aload_0
38: getfield #1 // Field metadata:Lorg/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata;
41: invokestatic #13 // Method org/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata.access$300:(Lorg/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata;)Lorg/springframework/cache/interceptor/CacheOperation;
44: invokevirtual #19 // Method org/springframework/cache/interceptor/CacheOperation.getCondition:()Ljava/lang/String;
47: aload_0
48: getfield #1 // Field metadata:Lorg/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata;
51: invokestatic #23 // Method org/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata.access$600:(Lorg/springframework/cache/interceptor/CacheAspectSupport$CacheOperationMetadata;)Lorg/springframework/context/expression/AnnotatedElementKey;
54: aload_2
55: invokevirtual #24 // Method org/springframework/cache/interceptor/CacheOperationExpressionEvaluator.condition:(Ljava/lang/String;Lorg/springframework/context/expression/AnnotatedElementKey;Lorg/springframework/expression/EvaluationContext;)Z
58: invokestatic #25 // Method java/lang/Boolean.valueOf:(Z)Ljava/lang/Boolean;
61: putfield #18 // Field conditionPassing:Ljava/lang/Boolean;
64: goto 75
67: aload_0
68: iconst_1
69: invokestatic #25 // Method java/lang/Boolean.valueOf:(Z)Ljava/lang/Boolean;
72: putfield #18 // Field conditionPassing:Ljava/lang/Boolean;
75: aload_0
76: getfield #18 // Field conditionPassing:Ljava/lang/Boolean;
79: invokevirtual #26 // Method java/lang/Boolean.booleanValue:()Z
82: ireturn
We see that auto-boxing occurs for entries 58 and 69 (not just for 69). So if we wanted to make such a change, we'd want to do it in both places.
But we also notice that there's auto-unboxing for entry 79 (the return statement).
In other words, there is actually quite a lot of automatic boxing and unboxing occurring throughout the code base, and we simply let the compiler and JVM optimize as much as possible for us.
In light of that, I am closing this PR.
However, if you determine via benchmarking (e.g., JMH) that this is somehow a hot spot in a normal Spring application, we might consider making a change.
Comment From: chenqimiao
Thank you for your reply. @sbrannen
Actually I am still a bit confused, we know that sometimes auto-boxing or auto-unboxing improve the convenience of program writing, but if it doesn’t work, why don't we minimize this conversion as much as possible? At least a little performance improvement.
Comment From: sbrannen
but if it doesn’t work,
What do you mean by "it doesn't work" in the context of the proposed change?
why don't we minimize this conversion as much as possible? At least a little performance improvement.
We generally don't like to optimize simply for the sake of optimization. Rather, we like to optimize if we know there is a significant improvement for a majority of use cases of the code in question (based on reliable benchmarks).
For the boolean boxing and unboxing involved here, the implementations are already streamlined. See the implementations of Boolean.valueOf(boolean) and Boolean.booleanValue() in the JDK for reference. No new objects are being created here: we are effectively always using the Boolean.TRUE and Boolean.FALSE singleton constants.
So, although there may be some small performance improvement associated with avoiding the boxing and unboxing as much as possible, the improvement is likely negligible in the grand scheme of things.
Comment From: chenqimiao
but if it doesn’t work,
I mean is that the use of auto-boxing here does not simplify code writing.
But now, I also understand what you mean, we should optimize based on benchmarks, not for optimization.
Thanks again for your reply.
@sbrannen