[MAGNOLIA-5904] Mycila-Guice dependency: Apply the provided patch to the Mycila version 2.10GA Created: 03/Sep/14  Updated: 10/Sep/14  Resolved: 10/Sep/14

Status: Closed
Project: Magnolia
Component/s: core
Affects Version/s: 5.2.7, 5.3.2
Fix Version/s: None

Type: Bug Priority: Critical
Reporter: Christian Ringele Assignee: Daniel Lipp
Resolution: Won't Fix Votes: 0
Labels: support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Java Source File Reflect.java     Text File com.mycila.inject.internal.Reflect.patch    
Issue Links:
Relates
relates to MAGNOLIA-5907 Update to latest mycila 3.5.ga Closed
causality
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Mycila is a java library for Google-Guice. The method following method was noticed to be blocking:

com.mycila.inject.internal.Reflect#annotatedBy(final Class<? extends Annotation> annotationType)

The method calls "java.lang.reflect.Method#declaredAnnotations()" which is synchronized. The results are not buffered. Mycila is currently used in Version 2.10GA as a dependency of Magnolia 5.3.1 Core

Work around:
Creating a patched version of Mycila 2.10GA and changing the calls of „java.lang.reflect.Method#declaredAnnotations()“ to „com.mycila.inject.internal.Reflect#annotatedBy(final Class<? extends Annotation> annotationType)“ by coating it with a cache.

Stacktrace of the code path can be found here https://github.com/mycila/guice/issues/6



 Comments   
Comment by Tobias Mattsson [ 05/Sep/14 ]

The cache needs to use both annotationType and the element to test as key. Right now its using only the annotationType and therefore it produces false cache hits.

It should also be mentioned that java.lang.reflect.Method#declaredAnnotations() is cached internally meaning that invocations after the first boils down to: acquire_mutex, null-check, return, release_mutex. Would be interesting to see performance comparisons of invoking getDeclaredAnnotations() directly versus using a ConcurrentHashMap based cache.

Comment by Tobias Mattsson [ 05/Sep/14 ]

The patch is ineffective because it caches only the predicate instances not the result after executing them. The same number of calls to isAnnotationPresent() are still made. It only adds overhead and no performance improvement.

I have measured the effect of caching and concluded that it is not worthwhile as long as calls happen on the same Method instance.

Method instances are not singletons within the JVM. Method instances are owned by the Class instance that created them. They're held (cached) for as long as the methods are not redefined, presumably through HotSpot code replace. New Class instances acquired through for instance Class.forName() have their own Method instances.

Mycila has a cache of Method instances in Reflect.java that it operates on so I see no value in applying this cache. The cache in java.lang.reflect.Method#declaredAnnotations() is more effective.

Comment by Daniel Lipp [ 10/Sep/14 ]

As the proposed patch doesn't solve the reported performance issue (see comments from Tobias) we won't apply it.

Generated at Mon Feb 12 04:09:29 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.