Hi, IMHO SLING-6584 is only about the race condition with set and reset. If we get rid of resetting the field accessibility this can no longer occur. Also the test which has been added in the context of SLING-6584 should show nicely that synchronisation is not necessary without reset. I am also +1 on the performance improvement. @Paul: Can you come up with a PR?
Thanks, Konrad > On 6. Dec 2021, at 10:20, Robert Munteanu <[email protected]> wrote: > > 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 >>>> >
