good point about SLING-6584 - here is the related commit [2]. the code that was in place before did the setAcccessible(false) without synchronization - this is indeed a threading issue as the Field reflection object is not thread-safe.
so our recommendation is to remove both synchronization and setAccessible(false). stefan [2] https://github.com/apache/sling-org-apache-sling-models-impl/commit/1d2bb13915352fd9d60775cfdab4e0afef945977 >-----Original Message----- >From: Robert Munteanu <[email protected]> >Sent: Monday, December 6, 2021 10:21 AM >To: [email protected] >Subject: Re: Discuss removing synchronized blocks from injection points >within sling-models-impl > >Hi, > >The injection seems to have been added with [1]. I think it's worth >checking if that issue is still a problem. > >Thanks, >Robert > >[1]: https://issues.apache.org/jira/browse/SLING-6584 > >On Mon, 2021-12-06 at 09:13 +0000, Stefan Seifert wrote: >> hello paul. >> >> thanks for doing all this testing. i also think it's not worth >> bothering to reset a field to non-accessible if it was set to >> accessible, and requiring a synchronization block for this. we should >> remove the synchronization and the setAccessible(false). it's a >> performance gain and simplifies the code. >> >> currently we have three places like this in the sling models impl code: >> https://github.com/apache/sling-org-apache-sling-models- >impl/search?q=setAccessible >> >> the question is if setAccessible(true) alone without synchronization >> may lead to any threading issues - some stack overflow articles and the >> actual felix code you mentioned seem to assure it's not a problem as >> long it's not reset to true again. >> >> stefan >> >> >> > -----Original Message----- >> > From: Paul Bjorkstrand <[email protected]> >> > Sent: Sunday, December 5, 2021 4:39 AM >> > To: [email protected] >> > Subject: Re: Discuss removing synchronized blocks from injection >> > points >> > within sling-models-impl >> > >> > I tried to create a JMH to show the difference between various >> > injection >> > techniques. While I am familiar with JMH and how to write them, I >> > make no >> > claims that this is perfect. Also, I did not do any kind of analysis, >> > such >> > as looking into the hot methods assembly. >> > >> > I made eight different benchmarks, covering the following: >> > >> > 1. Set field directly (control). >> > 2. Set field via setter method (alternate control). >> > 3. Set field via reflection using Field object, not synchronized. >> > 4. Set field via reflection using Method object for setter method, >> > not >> > synchronized. >> > 5. Set field via reflection using Field object, synchronized. >> > 6. Set field via reflection using Method object for setter method, >> > synchronized. >> > 7. Set field via MethodHandle using a handle to the field setter. >> > 8. Set field via MethodHandle using a handle to the setter method. >> > >> > You can see the full results & code at [1]. I only kept a few of the >> > best >> > (lowest error value) runs. One of the best runs I have copied below, >> > but >> > they are best viewed in monospace font, like at [1]. >> > >> > These numbers are specific to my machine, so the important inference >> > that >> > can be gained from these data is the relative, not absolute values. >> > >> > Benchmark (str) >> > Mode >> > Cnt Score Error Units >> > SynchronizedBenchmark.viaField str >> > avgt >> > 15 7.158 ± 0.181 ns/op >> > SynchronizedBenchmark.viaSetterMethod str >> > avgt >> > 15 7.802 ± 0.193 ns/op >> > SynchronizedBenchmark.viaMethodHandle_setterMethod str >> > avgt >> > 15 17.766 ± 0.551 ns/op >> > SynchronizedBenchmark.viaMethodHandle_field str >> > avgt >> > 15 17.939 ± 0.290 ns/op >> > SynchronizedBenchmark.viaReflection_field str >> > avgt >> > 15 157.453 ± 2.678 ns/op >> > SynchronizedBenchmark.viaReflection_setterMethod str >> > avgt >> > 15 157.346 ± 2.049 ns/op >> > SynchronizedBenchmark.viaReflection_setterMethod_synchronized str >> > avgt >> > 15 542.966 ± 19.825 ns/op >> > SynchronizedBenchmark.viaReflection_field_synchronized str >> > avgt >> > 15 771.381 ± 47.922 ns/op >> > >> > I added in using MethodHandles in the tests, just to show the >> > difference. >> > When Sling stops supporting Java 8, we can move to MethodHandle which >> > has >> > significantly better performance characteristics. Unfortunately, we >> > can't >> > make that move until we are on at least Java 9, so that we can use >> > MethodHandles.privateLookupIn(..) [2]. >> > >> > Regardless, you can see the significant difference between >> > synchronized vs >> > non-synchronized: synchronizing is 3-8x slower than not, when under >> > contention with multiple threads. >> > >> > [1]: >> > https://gist.github.com/paul- >bjorkstrand/f3bb154665e7d2168b4656eb7b794496 >> > [2]: >> > >https://docs.oracle.com/javase/9/docs/api/java/lang/invoke/MethodHandles.ht >> > ml#privateLookupIn-java.lang.Class- >> > java.lang.invoke.MethodHandles.Lookup- >> > -Paul >> > >> > >> > On Sat, Dec 4, 2021 at 9:48 AM Paul Bjorkstrand >> > <[email protected]> >> > wrote: >> > >> > > In light of discussion on a recently merged PR [1], I raised the >> > > question >> > > 'do we need to "reset" the accessible flag for reflective >> > > injectables >> > > (fields, methods, constructors)?' >> > > >> > > The sole purpose of synchronized around the injection logic >> > > (including >> > > constructor instantiation) seems to surround properly resetting the >> > > accessible flag on the reflection element, and ensuring the element >> > > is >> > > accessible during the injection. >> > > >> > > It is likely worth the effort to determine if resetting that flag, >> > > and >> > > thus the accompanying synchronized, is really needed. As a >> > > reference, I >> > > searched Apache Felix for uses of setAccessible, as it also needs >> > > to be >> > > able to inject into non-public elements [2]. After browsing a >> > > handful of >> > > the non-test classes, I found none that do anything more than >> > > setAccessible(true). This leads me to believe that we are doing >> > > extra >> > work >> > > in Sling Models that isn't really necessary. >> > > >> > > Removing synchronized could have a dramatic, positive impact on >> > > performance, especially for situations where there may be high >> > > contention >> > > (e.g. heavy model/submodel reuse, sharing a common super class, >> > > etc). If >> > > those synchronizations were removed, overall throughput will likely >> > > increase. >> > > >> > > Refs: >> > > [1]: >> > > https://github.com/apache/sling-org-apache-sling-models- >> > impl/pull/11/files#r762409049 >> > > [2]: https://github.com/apache/felix-dev/search?q=setAccessible >> > > >> > > -Paul >> > >
