You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by Oliver Heger <ol...@oliver-heger.de> on 2012/07/29 21:40:37 UTC
[configuration] Checkstyle warning about double-checked locking in
DynamicCombinedConfiguration
There is a checkstyle warning about double-checked locking in method
DynamicCombinedConfiguration.getCurrentConfig(). Indeed, the
double-check locking idiom is used, however, there is a comment saying
that this safe due to the usage of a ConcurrentMap.
This may be true, but I wonder whether it would be better to use the
map's putIfAbsent() method and avoid synchronization. The worst thing
that can happen is that on parallel access multiple
CombinedConfiguration instances are created which can be passed
immediately to the garbage collector.
Thoughts?
Oliver
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [configuration] Checkstyle warning about double-checked locking
in DynamicCombinedConfiguration
Posted by sebb <se...@gmail.com>.
On 30 July 2012 22:49, Ralph Goers <ra...@dslextreme.com> wrote:
>
> On Jul 30, 2012, at 2:17 PM, sebb wrote:
>
>> On 30 July 2012 22:13, Ralph Goers <ra...@dslextreme.com> wrote:
>>>
>>> On Jul 30, 2012, at 2:10 PM, sebb wrote:
>>>
>>>> On 30 July 2012 20:34, Oliver Heger <ol...@oliver-heger.de> wrote:
>>>>> Am 29.07.2012 22:41, schrieb Ralph Goers:
>>>>>
>>>>>> I used that specifically to avoid creating multiple combined
>>>>>> configurations since it can be fairly expensive to create them. I looked
>>>>>> into the guarantees that ConcurrentMap provides before I implemented that
>>>>>> and included the comment since I knew it would catch someone's eye.
>>>>>>
>>>>>> Ralph
>>>>>
>>>>>
>>>>> Thanks for clarifying. I added a suppression in the Checkstyle configuration
>>>>> so that this warning will not pop up again.
>>>>
>>>> An alternative solution might be to drop the synchronised block and
>>>> use the atomic putIfAbsent() method.
>>>>
>>>> That would reduce the synch. overhead.
>>>> The downside is that the code might occasionally create and configure
>>>> an instance of CombinedConfiguration that is then not used (if another
>>>> thread happens to do the same).
>>>
>>> Did you read Oliver's message which is directly below what you wrote?
>>
>> Yes, but that only talks about suppressing the warning, not reducing
>> the amount of time spent in the synchronised block.
>
> We must be reading something different. In the second paragraph below Oliver also suggested using putIfAbsent and removing the synchronization. My response at the top of this is exactly why I chose not to do that.
Sorry, I see now. I read the code and the last part of the thread (only).
It would be helpful to add a comment to the code as to why putIfAbsent
was not used so this is not raised again.
> Ralph
>
>>
>>> Ralph
>>>
>>>>
>>>>> Oliver
>>>>>
>>>>>
>>>>>>
>>>>>> On Jul 29, 2012, at 12:40 PM, Oliver Heger wrote:
>>>>>>
>>>>>>> There is a checkstyle warning about double-checked locking in method
>>>>>>> DynamicCombinedConfiguration.getCurrentConfig(). Indeed, the double-check
>>>>>>> locking idiom is used, however, there is a comment saying that this safe due
>>>>>>> to the usage of a ConcurrentMap.
>>>>>>>
>>>>>>> This may be true, but I wonder whether it would be better to use the
>>>>>>> map's putIfAbsent() method and avoid synchronization. The worst thing that
>>>>>>> can happen is that on parallel access multiple CombinedConfiguration
>>>>>>> instances are created which can be passed immediately to the garbage
>>>>>>> collector.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>>> Oliver
>>>>>>>
>>>>>>> ---------------------------------------------------------------------
>>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>>
>>>>>>
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [configuration] Checkstyle warning about double-checked locking in DynamicCombinedConfiguration
Posted by Ralph Goers <ra...@dslextreme.com>.
On Jul 30, 2012, at 2:17 PM, sebb wrote:
> On 30 July 2012 22:13, Ralph Goers <ra...@dslextreme.com> wrote:
>>
>> On Jul 30, 2012, at 2:10 PM, sebb wrote:
>>
>>> On 30 July 2012 20:34, Oliver Heger <ol...@oliver-heger.de> wrote:
>>>> Am 29.07.2012 22:41, schrieb Ralph Goers:
>>>>
>>>>> I used that specifically to avoid creating multiple combined
>>>>> configurations since it can be fairly expensive to create them. I looked
>>>>> into the guarantees that ConcurrentMap provides before I implemented that
>>>>> and included the comment since I knew it would catch someone's eye.
>>>>>
>>>>> Ralph
>>>>
>>>>
>>>> Thanks for clarifying. I added a suppression in the Checkstyle configuration
>>>> so that this warning will not pop up again.
>>>
>>> An alternative solution might be to drop the synchronised block and
>>> use the atomic putIfAbsent() method.
>>>
>>> That would reduce the synch. overhead.
>>> The downside is that the code might occasionally create and configure
>>> an instance of CombinedConfiguration that is then not used (if another
>>> thread happens to do the same).
>>
>> Did you read Oliver's message which is directly below what you wrote?
>
> Yes, but that only talks about suppressing the warning, not reducing
> the amount of time spent in the synchronised block.
We must be reading something different. In the second paragraph below Oliver also suggested using putIfAbsent and removing the synchronization. My response at the top of this is exactly why I chose not to do that.
Ralph
>
>> Ralph
>>
>>>
>>>> Oliver
>>>>
>>>>
>>>>>
>>>>> On Jul 29, 2012, at 12:40 PM, Oliver Heger wrote:
>>>>>
>>>>>> There is a checkstyle warning about double-checked locking in method
>>>>>> DynamicCombinedConfiguration.getCurrentConfig(). Indeed, the double-check
>>>>>> locking idiom is used, however, there is a comment saying that this safe due
>>>>>> to the usage of a ConcurrentMap.
>>>>>>
>>>>>> This may be true, but I wonder whether it would be better to use the
>>>>>> map's putIfAbsent() method and avoid synchronization. The worst thing that
>>>>>> can happen is that on parallel access multiple CombinedConfiguration
>>>>>> instances are created which can be passed immediately to the garbage
>>>>>> collector.
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>> Oliver
>>>>>>
>>>>>> ---------------------------------------------------------------------
>>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [configuration] Checkstyle warning about double-checked locking
in DynamicCombinedConfiguration
Posted by sebb <se...@gmail.com>.
On 30 July 2012 22:13, Ralph Goers <ra...@dslextreme.com> wrote:
>
> On Jul 30, 2012, at 2:10 PM, sebb wrote:
>
>> On 30 July 2012 20:34, Oliver Heger <ol...@oliver-heger.de> wrote:
>>> Am 29.07.2012 22:41, schrieb Ralph Goers:
>>>
>>>> I used that specifically to avoid creating multiple combined
>>>> configurations since it can be fairly expensive to create them. I looked
>>>> into the guarantees that ConcurrentMap provides before I implemented that
>>>> and included the comment since I knew it would catch someone's eye.
>>>>
>>>> Ralph
>>>
>>>
>>> Thanks for clarifying. I added a suppression in the Checkstyle configuration
>>> so that this warning will not pop up again.
>>
>> An alternative solution might be to drop the synchronised block and
>> use the atomic putIfAbsent() method.
>>
>> That would reduce the synch. overhead.
>> The downside is that the code might occasionally create and configure
>> an instance of CombinedConfiguration that is then not used (if another
>> thread happens to do the same).
>
> Did you read Oliver's message which is directly below what you wrote?
Yes, but that only talks about suppressing the warning, not reducing
the amount of time spent in the synchronised block.
> Ralph
>
>>
>>> Oliver
>>>
>>>
>>>>
>>>> On Jul 29, 2012, at 12:40 PM, Oliver Heger wrote:
>>>>
>>>>> There is a checkstyle warning about double-checked locking in method
>>>>> DynamicCombinedConfiguration.getCurrentConfig(). Indeed, the double-check
>>>>> locking idiom is used, however, there is a comment saying that this safe due
>>>>> to the usage of a ConcurrentMap.
>>>>>
>>>>> This may be true, but I wonder whether it would be better to use the
>>>>> map's putIfAbsent() method and avoid synchronization. The worst thing that
>>>>> can happen is that on parallel access multiple CombinedConfiguration
>>>>> instances are created which can be passed immediately to the garbage
>>>>> collector.
>>>>>
>>>>> Thoughts?
>>>>>
>>>>> Oliver
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [configuration] Checkstyle warning about double-checked locking in DynamicCombinedConfiguration
Posted by Ralph Goers <ra...@dslextreme.com>.
On Jul 30, 2012, at 2:10 PM, sebb wrote:
> On 30 July 2012 20:34, Oliver Heger <ol...@oliver-heger.de> wrote:
>> Am 29.07.2012 22:41, schrieb Ralph Goers:
>>
>>> I used that specifically to avoid creating multiple combined
>>> configurations since it can be fairly expensive to create them. I looked
>>> into the guarantees that ConcurrentMap provides before I implemented that
>>> and included the comment since I knew it would catch someone's eye.
>>>
>>> Ralph
>>
>>
>> Thanks for clarifying. I added a suppression in the Checkstyle configuration
>> so that this warning will not pop up again.
>
> An alternative solution might be to drop the synchronised block and
> use the atomic putIfAbsent() method.
>
> That would reduce the synch. overhead.
> The downside is that the code might occasionally create and configure
> an instance of CombinedConfiguration that is then not used (if another
> thread happens to do the same).
Did you read Oliver's message which is directly below what you wrote?
Ralph
>
>> Oliver
>>
>>
>>>
>>> On Jul 29, 2012, at 12:40 PM, Oliver Heger wrote:
>>>
>>>> There is a checkstyle warning about double-checked locking in method
>>>> DynamicCombinedConfiguration.getCurrentConfig(). Indeed, the double-check
>>>> locking idiom is used, however, there is a comment saying that this safe due
>>>> to the usage of a ConcurrentMap.
>>>>
>>>> This may be true, but I wonder whether it would be better to use the
>>>> map's putIfAbsent() method and avoid synchronization. The worst thing that
>>>> can happen is that on parallel access multiple CombinedConfiguration
>>>> instances are created which can be passed immediately to the garbage
>>>> collector.
>>>>
>>>> Thoughts?
>>>>
>>>> Oliver
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [configuration] Checkstyle warning about double-checked locking
in DynamicCombinedConfiguration
Posted by sebb <se...@gmail.com>.
On 30 July 2012 20:34, Oliver Heger <ol...@oliver-heger.de> wrote:
> Am 29.07.2012 22:41, schrieb Ralph Goers:
>
>> I used that specifically to avoid creating multiple combined
>> configurations since it can be fairly expensive to create them. I looked
>> into the guarantees that ConcurrentMap provides before I implemented that
>> and included the comment since I knew it would catch someone's eye.
>>
>> Ralph
>
>
> Thanks for clarifying. I added a suppression in the Checkstyle configuration
> so that this warning will not pop up again.
An alternative solution might be to drop the synchronised block and
use the atomic putIfAbsent() method.
That would reduce the synch. overhead.
The downside is that the code might occasionally create and configure
an instance of CombinedConfiguration that is then not used (if another
thread happens to do the same).
> Oliver
>
>
>>
>> On Jul 29, 2012, at 12:40 PM, Oliver Heger wrote:
>>
>>> There is a checkstyle warning about double-checked locking in method
>>> DynamicCombinedConfiguration.getCurrentConfig(). Indeed, the double-check
>>> locking idiom is used, however, there is a comment saying that this safe due
>>> to the usage of a ConcurrentMap.
>>>
>>> This may be true, but I wonder whether it would be better to use the
>>> map's putIfAbsent() method and avoid synchronization. The worst thing that
>>> can happen is that on parallel access multiple CombinedConfiguration
>>> instances are created which can be passed immediately to the garbage
>>> collector.
>>>
>>> Thoughts?
>>>
>>> Oliver
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>>> For additional commands, e-mail: dev-help@commons.apache.org
>>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [configuration] Checkstyle warning about double-checked locking
in DynamicCombinedConfiguration
Posted by Oliver Heger <ol...@oliver-heger.de>.
Am 29.07.2012 22:41, schrieb Ralph Goers:
> I used that specifically to avoid creating multiple combined configurations since it can be fairly expensive to create them. I looked into the guarantees that ConcurrentMap provides before I implemented that and included the comment since I knew it would catch someone's eye.
>
> Ralph
Thanks for clarifying. I added a suppression in the Checkstyle
configuration so that this warning will not pop up again.
Oliver
>
> On Jul 29, 2012, at 12:40 PM, Oliver Heger wrote:
>
>> There is a checkstyle warning about double-checked locking in method DynamicCombinedConfiguration.getCurrentConfig(). Indeed, the double-check locking idiom is used, however, there is a comment saying that this safe due to the usage of a ConcurrentMap.
>>
>> This may be true, but I wonder whether it would be better to use the map's putIfAbsent() method and avoid synchronization. The worst thing that can happen is that on parallel access multiple CombinedConfiguration instances are created which can be passed immediately to the garbage collector.
>>
>> Thoughts?
>>
>> Oliver
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
>> For additional commands, e-mail: dev-help@commons.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: [configuration] Checkstyle warning about double-checked locking in DynamicCombinedConfiguration
Posted by Ralph Goers <ra...@dslextreme.com>.
I used that specifically to avoid creating multiple combined configurations since it can be fairly expensive to create them. I looked into the guarantees that ConcurrentMap provides before I implemented that and included the comment since I knew it would catch someone's eye.
Ralph
On Jul 29, 2012, at 12:40 PM, Oliver Heger wrote:
> There is a checkstyle warning about double-checked locking in method DynamicCombinedConfiguration.getCurrentConfig(). Indeed, the double-check locking idiom is used, however, there is a comment saying that this safe due to the usage of a ConcurrentMap.
>
> This may be true, but I wonder whether it would be better to use the map's putIfAbsent() method and avoid synchronization. The worst thing that can happen is that on parallel access multiple CombinedConfiguration instances are created which can be passed immediately to the garbage collector.
>
> Thoughts?
>
> Oliver
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org