RFR JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe

[Claes Redestad]

Hi,

I think this looks good, but since these WeakPairMaps won't be used for 
many applications I wonder if it's worth keeping the implementation 
lazy, for example by moving the maps to a holder class:

http://cr.openjdk.java.net/~redestad/scratch/transients.01/

I verified this avoids loading the WeakPairMap class for various small 
programs and should be neutral in other regards.

Thanks!

/Claes

On 2016-04-25 13:13, Peter Levart wrote:
> Hi Alan,
>
> I created an issue for this:
>
>     JDK-8155005: java.lang.reflect.Module.WeakSet is not thread-safe
>
>     https://bugs.openjdk.java.net/browse/JDK-8155005
>
>
> I did what you suggested, renamed type parameters to <K1, K2, V>, 
> split long line in Module and modified the test so that it now waits 
> for entry to be expunged for up to 5 seconds when it is expected to be 
> expunged but returns as soon as it detects that the entry is gone 
> (usually in 1st 100 ms). It still waits just 500 ms when the entry is 
> expected to remain in the map. False negatives should be eliminated 
> this way and the test still doesn't waist plenty of execution time.
>
> Here's latest webrev:
>
> http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.04/ 
>
>
>
> If this is accepted then I would need to know via which repo this 
> should be pushed (jdk9/dev or jake).
>
> Regards, Peter
>
>
> On 04/24/2016 10:16 AM, Alan Bateman wrote:
>>
>> On 22/04/2016 14:42, Peter Levart wrote:
>>> :
>>>
>>> I tried to reduce the complexity of WeakPairMap as much as I could. 
>>> I added some docs that describe the architecture. Hopefully this is 
>>> now easier to grasp:
>>>
>>> http://cr.openjdk.java.net/~plevart/jdk9-dev/Module.WeakSet.multithreadUnsafe/webrev.03/ 
>>>
>> I think this looks quite good.
>>
>> As there are two keys then what would you think about renaming the 
>> type parameters to <K1, K2, V>?
>>
>> In the test when I wonder if 500ms is enough to guarantee that 
>> references has been queued, esp. on loaded machines, maybe a previous 
>> test has several something loop for example.
>>
>> In terms of the startup then only WeakPairMap should be loaded so I 
>> think that is okay. We'll find that -XaddExports will be a bit more 
>> expensive as will trigger quite a bit of initialization and class 
>> loading but I think that is okay too.
>>
>> A minor comment on Module L518 is that we might want to split this 
>> line to avoid it being too long. Alternatively, just replace 
>> exportedToAll, exportedToOther and exportedToAllUnnamed with "exports".
>>
>>>
>>> Another possibility for transientReads and transientExports (but not 
>>> for transientUses) could be if each instance of Module object held a 
>>> unique long id allocated at its creation from say AtomicLong. You 
>>> could then construct maps with Long ids instead of Module(s), but 
>>> that has a drawback that some Module (say a system module or a 
>>> module of an app server) could accumulate ids from modules long gone 
>>> (for example when some app in an app server is redeployed multiple 
>>> times) so this would be a memory leak...
>> I agree, particularly with exports (not reads, at least not anymore).
>>
>>>
>>> a single global WeakPairMap has an advantage over individual 
>>> WeakSet(s) per Module instance because it is accessed more 
>>> frequently so expunging of stale entries happens more promptly.
>> True. Another advantage is that it drops 3 instance fields.
>>
>> -Alan.
>