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.
