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