[cdi-dev] Mutable annotation literals

[Stuart Douglas]

I don't actually think this is an issue any more, and I think we should just reject it. 

To see why have a look at sun.reflect.annotation.AnnotationInvocationHandler#hashCodeImpl, 
basically the sun JDK still uses reflection and no caching for hashCode() and equals() for all annotations, 
as the number of JDK generated annotations will likely dwarf the number of AnnotationLiterals, and as
there are concerns about mutable annotations I think we should just drop it. In practice I doubt it will make
any noticeable difference.

Weld used to use annotations directly as map keys, however we don't do this any more, simply because
they are so slow. 

Stuart

On 16/08/2012, at 11:09 PM, Mark Struberg <struberg at yahoo.de> wrote:

>> Except that the hash code rules for annotations aren't that easy to follow.
> Exactly! That was the reason why I made the sample with the super.hashCode() code.
> 
> For any 'constant' AnnotationLiteral you just additionally add a few lines which 'caches' the hashCode.
> 
> 
> 
> public class TransactionScopedLiteral extends AnnotationLiteral<TransactionScoped> implements TransactionScoped {
>   private Integer hashCode = null;
>   
> 
>   public int hashCode() {
>     if (hashCode == null) {
> 
>       hashCode = super.hashCode();
>    }
> 
>     return hashCode;
>   }
>   
> 
>   ...
> 
> }
> 
> 
> we could of course also add a new additional constructor with a 'boolean constant' parameter
> 
> LieGrue,
> strub
> 
> 
> 
> ----- Original Message -----
>> From: Pete Muir <pmuir at redhat.com>
>> To: Mark Struberg <struberg at yahoo.de>
>> Cc: cdi-dev <cdi-dev at lists.jboss.org>
>> Sent: Thursday, August 16, 2012 3:01 PM
>> Subject: Re: [cdi-dev] Mutable annotation literals
>> 
>> 
>> On 16 Aug 2012, at 13:58, Mark Struberg wrote:
>> 
>>> The problem is an AnnotationLiteral could theoretically be mutable as well. 
>> Thus the AnnotationLiteral class must not make such assumptions imo. 
>> 
>> Agreed.
>> 
>>> 
>>> 
>>> I the case of an empty value-methods list we could btw also improve the 
>> equals method.
>> 
>> Yes.
>> 
>>> 
>>> In the case where you have value-methods, you already need to implement the 
>> interface manually. It would be no problem to just add the 2 lines for caching 
>> the hashCode in case the values do not change.
>> 
>> Except that the hash code rules for annotations aren't that easy to follow.
>> 
>>> 
>>> 
>>> LieGrue,
>>> strub
>>> 
>>> 
>>> 
>>> ----- Original Message -----
>>>> From: Pete Muir <pmuir at redhat.com>
>>>> To: Mark Struberg <struberg at yahoo.de>
>>>> Cc: cdi-dev <cdi-dev at lists.jboss.org>
>>>> Sent: Thursday, August 16, 2012 2:20 PM
>>>> Subject: Re: [cdi-dev] Mutable annotation literals
>>>> 
>>>> I guess what Marko/I am proposing is that we provide a utility for the 
>> common 
>>>> case of an immutable annotation literal with member values...
>>>> 
>>>> On 16 Aug 2012, at 13:18, Mark Struberg wrote:
>>>> 
>>>>> Imo we don't need to cache the hashCode in the 
>> AnnotationLiteral 
>>>> itself. We just need to cache if there are value methods or not. If 
>> not, then we 
>>>> can immediately return 0. If there are value methods present, then you 
>> need to 
>>>> extend the AnnotationLiteral class anyway and can add an own hashCode 
>> which 
>>>> delegates to super.hashCode() and caches the result if the values will 
>> not be 
>>>> changed.
>>>>> 
>>>>> LieGrue,
>>>>> strub
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> ----- Original Message -----
>>>>>> From: Pete Muir <pmuir at redhat.com>
>>>>>> To: cdi-dev <cdi-dev at lists.jboss.org>
>>>>>> Cc: 
>>>>>> Sent: Thursday, August 16, 2012 1:43 PM
>>>>>> Subject: [cdi-dev] Mutable annotation literals
>>>>>> 
>>>>>> All
>>>>>> 
>>>>>> Marko has added caching for the hashcode value on 
>> AnnotationLiteral, 
>>>> which is 
>>>>>> based on the premise that an AnnotationLiteral is not mutable.
>>>>>> 
>>>>>> 
>>>> 
>> https://github.com/luksa/cdi/commit/f3c200fcdf8ca8681c194c921567aedcce375102
>>>>>> 
>>>>>> This is a reasonable thing to require (as real annotations are 
>> normally 
>>>> 
>>>>>> immutable!) and a very good optimisation, however it is not 
>> backwards 
>>>>>> compatible, as we have not previously required 
>> AnnotationLiteral to be 
>>>>>> immutable.
>>>>>> 
>>>>>> An alternative option is to introduce the 
>> ImmutableAnnotationLiteral 
>>>> subclass, 
>>>>>> which would require any concrete instances to be immutable and 
>> could 
>>>> use this 
>>>>>> optimisation. We could "deprecate" AnnotationLiteral 
>> (as 
>>>> described in 
>>>>>> java.net/projects/javaee-spec/pages/CompatibilityRequirements).
>>>>>> 
>>>>>> Note that regardless, we can apply the minor optimisation 
>> suggested by 
>>>> Mark, 
>>>>>> that we return 0 if the annotation has no members.
>>>>>> 
>>>>>> Pete
>>>>>> _______________________________________________
>>>>>> cdi-dev mailing list
>>>>>> cdi-dev at lists.jboss.org
>>>>>> https://lists.jboss.org/mailman/listinfo/cdi-dev
>>>>>> 
>>>> 
>> 
> 
> _______________________________________________
> cdi-dev mailing list
> cdi-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/cdi-dev