You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by Paul Bjorkstrand <pa...@gmail.com> on 2021/12/04 15:48:50 UTC
Discuss removing synchronized blocks from injection points within sling-models-impl
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
Re: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Paul Bjorkstrand <pa...@gmail.com>.
Agreed, Konrad. That was the way I was going to approach it. I am adding
the "remove sync" ticket now, and will add the MethodHandles ticket after I
have the PR for sync removal in, since it is far simpler.
-Paul
On Mon, Dec 6, 2021 at 9:07 AM Konrad Windszus <ko...@gmx.de> wrote:
> Maybe we can start with the simple code by removing synchronized and
> reset.
> Using MethodHandle should be treated separately (in a dedicated ticket and
> PR).
>
> Konrad
>
> > On 6. Dec 2021, at 16:03, Paul Bjorkstrand <pa...@gmail.com>
> wrote:
> >
> > It may be possible to do that, by doing a reflective check on that method
> > and calling that method reflectively (or via method handles, since it is
> > public). The code might be a bit complex, but I'll give it a go.
> >
> > -Paul
> >
> >
> > On Mon, Dec 6, 2021 at 3:22 AM Robert Munteanu <ro...@apache.org>
> wrote:
> >
> >> Hi Paul,
> >>
> >> On Sat, 2021-12-04 at 21:38 -0600, Paul Bjorkstrand wrote:
> >>> 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].
> >>
> >> Thanks for the comprehensive tests. Would it be possible to try
> >> MethodHandles and fall back to reflection is they're not available?
> >>
> >> Thanks,
> >> Robert
> >>
>
>
Re: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Konrad Windszus <ko...@gmx.de>.
Maybe we can start with the simple code by removing synchronized and reset.
Using MethodHandle should be treated separately (in a dedicated ticket and PR).
Konrad
> On 6. Dec 2021, at 16:03, Paul Bjorkstrand <pa...@gmail.com> wrote:
>
> It may be possible to do that, by doing a reflective check on that method
> and calling that method reflectively (or via method handles, since it is
> public). The code might be a bit complex, but I'll give it a go.
>
> -Paul
>
>
> On Mon, Dec 6, 2021 at 3:22 AM Robert Munteanu <ro...@apache.org> wrote:
>
>> Hi Paul,
>>
>> On Sat, 2021-12-04 at 21:38 -0600, Paul Bjorkstrand wrote:
>>> 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].
>>
>> Thanks for the comprehensive tests. Would it be possible to try
>> MethodHandles and fall back to reflection is they're not available?
>>
>> Thanks,
>> Robert
>>
Re: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Paul Bjorkstrand <pa...@gmail.com>.
It may be possible to do that, by doing a reflective check on that method
and calling that method reflectively (or via method handles, since it is
public). The code might be a bit complex, but I'll give it a go.
-Paul
On Mon, Dec 6, 2021 at 3:22 AM Robert Munteanu <ro...@apache.org> wrote:
> Hi Paul,
>
> On Sat, 2021-12-04 at 21:38 -0600, Paul Bjorkstrand wrote:
> > 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].
>
> Thanks for the comprehensive tests. Would it be possible to try
> MethodHandles and fall back to reflection is they're not available?
>
> Thanks,
> Robert
>
Re: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Robert Munteanu <ro...@apache.org>.
Hi Paul,
On Sat, 2021-12-04 at 21:38 -0600, Paul Bjorkstrand wrote:
> 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].
Thanks for the comprehensive tests. Would it be possible to try
MethodHandles and fall back to reflection is they're not available?
Thanks,
Robert
Re: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Konrad Windszus <ko...@gmx.de>.
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 <ro...@apache.org> 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 <pa...@gmail.com>
>>> Sent: Sunday, December 5, 2021 4:39 AM
>>> To: dev@sling.apache.org
>>> 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
>>> <pa...@gmail.com>
>>> 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
>>>>
>
Re: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Robert Munteanu <ro...@apache.org>.
On Mon, 2021-12-06 at 10:20 +0100, Robert Munteanu wrote:
> The injection
I meant the synchronisation.
RE: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Stefan Seifert <St...@diva-e.com.INVALID>.
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 <ro...@apache.org>
>Sent: Monday, December 6, 2021 10:21 AM
>To: dev@sling.apache.org
>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 <pa...@gmail.com>
>> > Sent: Sunday, December 5, 2021 4:39 AM
>> > To: dev@sling.apache.org
>> > 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
>> > <pa...@gmail.com>
>> > 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
>> > >
Re: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Robert Munteanu <ro...@apache.org>.
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 <pa...@gmail.com>
> > Sent: Sunday, December 5, 2021 4:39 AM
> > To: dev@sling.apache.org
> > 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
> > <pa...@gmail.com>
> > 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
> > >
RE: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Stefan Seifert <St...@diva-e.com.INVALID>.
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 <pa...@gmail.com>
>Sent: Sunday, December 5, 2021 4:39 AM
>To: dev@sling.apache.org
>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
><pa...@gmail.com>
>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
>>
Re: Discuss removing synchronized blocks from injection points within sling-models-impl
Posted by Paul Bjorkstrand <pa...@gmail.com>.
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.html#privateLookupIn-java.lang.Class-java.lang.invoke.MethodHandles.Lookup-
-Paul
On Sat, Dec 4, 2021 at 9:48 AM Paul Bjorkstrand <pa...@gmail.com>
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
>