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 > > >
