Re: Promptly freeing the per-thread cached direct buffers when a thread exits
Can I also add that, when a buffer in the cache needs to be replaced with a new (and typically larger) one, the old buffer is explicitly freed. So, the code already assumes that buffers that are in the cache should not be reachable from anywhere else. Explicitly freeing them when the thread exits is consistent (IMHO) with this behavior. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 22, 2018 at 7:11:05 AM, Alan Bateman (alan.bate...@oracle.com) wrote: On 21/06/2018 21:13, Florian Weimer wrote: > * Tony Printezis: > >> There are a few obvious ways to mitigate this, e.g., cause a Full GC / >> concurrent GC cycle at regular intervals. However, the best solution IMHO >> is to explicitly free any direct buffers that are still in the cache when a >> thread exits. > Why is this safe? Couldn't these direct byte buffers be used with a > custom channel that leaks them to another thread? The temporary direct buffer mechanism is internal to java.base so it should never be used with custom channel implementations. There may be some pre-existing issues with the FileChannel transferXXX methods when called with an untrusted source/sink that will need to be audited but this is not something that I can discuss on this mailing list. -Alan
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Peter, The changes to TestMaxCachedBufferSize.java look fine. One point though: Why do you need the TmpDirectBuffersReclamation.java test? In TestMaxCachedBufferSize.java you just call checkDirectBuffers(0, 0); after you the main thread calls join() on the workers? Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 1:29:38 PM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/21/2018 07:01 PM, Tony Printezis wrote: I’m trying exactly that. :-) Sorry, I didn't know. Here's my attempt: http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.07/ I also added @run main/othervm to TempDirectBuffersReclamation test. Peter — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 12:59:58 PM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/21/2018 06:17 PM, Tony Printezis wrote: I was saying: I looked at TestMaxCachedBufferSize and, unfortunately, I don’t think the test makes a lot of sense right now as it checks the number / size of direct buffers after all the threads terminate and, with this change, that should always be 0. You're right. The test makes no sense now. As I understand, the test checked that number/size of allocated temporary buffers did not exceed the estimated calculated size by checking the MXBean immediately after threads terminate. This will never happen after the change as they will all be freed before checking, regardless of how many were and how much was allocated. Perhaps the test should be modified to include a latch so that threads wait until the measurement is made and only then are allowed to exit... Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
I’m trying exactly that. :-) — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 12:59:58 PM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/21/2018 06:17 PM, Tony Printezis wrote: I was saying: I looked at TestMaxCachedBufferSize and, unfortunately, I don’t think the test makes a lot of sense right now as it checks the number / size of direct buffers after all the threads terminate and, with this change, that should always be 0. You're right. The test makes no sense now. As I understand, the test checked that number/size of allocated temporary buffers did not exceed the estimated calculated size by checking the MXBean immediately after threads terminate. This will never happen after the change as they will all be freed before checking, regardless of how many were and how much was allocated. Perhaps the test should be modified to include a latch so that threads wait until the measurement is made and only then are allowed to exit... Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
…and I also hadn’t attached the test. Sorry, I’m clearly very distracted today! — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 12:17:57 PM, Tony Printezis (tprinte...@twitter.com) wrote: (I unfortunately pressed Send accidentally; apologies) I was saying: I looked at TestMaxCachedBufferSize and, unfortunately, I don’t think the test makes a lot of sense right now as it checks the number / size of direct buffers after all the threads terminate and, with this change, that should always be 0. Let me look into this and I’ll get back to you in a bit. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 12:14:24 PM, Tony Printezis (tprinte...@twitter.com) wrote: Peter, Attached TerminatingThreadLocalTest.java. Let me know what you think (and feel free to modify it / discard it if you don’t like it!). Re: The test for the max cached buffer size is: test/jdk/sun/nio/ch/TestMaxCachedBufferSize.java. I looked at it and, unfortunately, — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 7:07:59 AM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/20/2018 04:24 PM, Tony Printezis wrote: Hey Peter, I had written a test to test my version of the exit hooks. I can easily rework it to work with yours. Interested? Of course. Just give what you got. I can modify it as needed. To check that the native buffers are reclaimed promptly, we should be able to amend the test that tests the setting of the max size for the cached buffers (i.e., check that, after all the threads have exited, the total native count / size is 0). Good idea. Do you happen to know which one is it? Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 20, 2018 at 10:08:48 AM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/18/2018 05:41 PM, Alan Bateman wrote: > > > On 17/06/2018 22:20, Peter Levart wrote: >> Update: the discussion on concurrent-interest about possible future >> addition of public ThreadLocal.getIfPresent() or >> ThreadLocal.isPresent() has died out, but it nevertheless reached a >> point where ThreadLocal.isPresent() was found the least problematic. >> So I would like to get further with this proposal using the last >> webrev.04 version of the patch that uses ThreadLocal.isPresent() >> internally - still package-private at the moment. >> >> Here's the webrev (unchanged from the day it was published): >> >> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/ >> >> Would this version be OK? > I think looks quite good. > > One small nit is that the update to ThreadLocal.setInitialValue makes > it look like all locals are registered when setting the initial value. > What would you think about moving the instanceof check from > TerminatingThreadLocal.register so that it's a bit more obvious. > > -Alan Like the following? http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.05/ Do we need a test which proves that cached direct buffers are released when thread exits or would a unit test for TerminatingThreadLocal be enough? Maybe both? Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
(I unfortunately pressed Send accidentally; apologies) I was saying: I looked at TestMaxCachedBufferSize and, unfortunately, I don’t think the test makes a lot of sense right now as it checks the number / size of direct buffers after all the threads terminate and, with this change, that should always be 0. Let me look into this and I’ll get back to you in a bit. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 12:14:24 PM, Tony Printezis (tprinte...@twitter.com) wrote: Peter, Attached TerminatingThreadLocalTest.java. Let me know what you think (and feel free to modify it / discard it if you don’t like it!). Re: The test for the max cached buffer size is: test/jdk/sun/nio/ch/TestMaxCachedBufferSize.java. I looked at it and, unfortunately, — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 7:07:59 AM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/20/2018 04:24 PM, Tony Printezis wrote: Hey Peter, I had written a test to test my version of the exit hooks. I can easily rework it to work with yours. Interested? Of course. Just give what you got. I can modify it as needed. To check that the native buffers are reclaimed promptly, we should be able to amend the test that tests the setting of the max size for the cached buffers (i.e., check that, after all the threads have exited, the total native count / size is 0). Good idea. Do you happen to know which one is it? Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 20, 2018 at 10:08:48 AM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/18/2018 05:41 PM, Alan Bateman wrote: > > > On 17/06/2018 22:20, Peter Levart wrote: >> Update: the discussion on concurrent-interest about possible future >> addition of public ThreadLocal.getIfPresent() or >> ThreadLocal.isPresent() has died out, but it nevertheless reached a >> point where ThreadLocal.isPresent() was found the least problematic. >> So I would like to get further with this proposal using the last >> webrev.04 version of the patch that uses ThreadLocal.isPresent() >> internally - still package-private at the moment. >> >> Here's the webrev (unchanged from the day it was published): >> >> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/ >> >> Would this version be OK? > I think looks quite good. > > One small nit is that the update to ThreadLocal.setInitialValue makes > it look like all locals are registered when setting the initial value. > What would you think about moving the instanceof check from > TerminatingThreadLocal.register so that it's a bit more obvious. > > -Alan Like the following? http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.05/ Do we need a test which proves that cached direct buffers are released when thread exits or would a unit test for TerminatingThreadLocal be enough? Maybe both? Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Peter, Attached TerminatingThreadLocalTest.java. Let me know what you think (and feel free to modify it / discard it if you don’t like it!). Re: The test for the max cached buffer size is: test/jdk/sun/nio/ch/TestMaxCachedBufferSize.java. I looked at it and, unfortunately, — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 21, 2018 at 7:07:59 AM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/20/2018 04:24 PM, Tony Printezis wrote: Hey Peter, I had written a test to test my version of the exit hooks. I can easily rework it to work with yours. Interested? Of course. Just give what you got. I can modify it as needed. To check that the native buffers are reclaimed promptly, we should be able to amend the test that tests the setting of the max size for the cached buffers (i.e., check that, after all the threads have exited, the total native count / size is 0). Good idea. Do you happen to know which one is it? Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 20, 2018 at 10:08:48 AM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/18/2018 05:41 PM, Alan Bateman wrote: > > > On 17/06/2018 22:20, Peter Levart wrote: >> Update: the discussion on concurrent-interest about possible future >> addition of public ThreadLocal.getIfPresent() or >> ThreadLocal.isPresent() has died out, but it nevertheless reached a >> point where ThreadLocal.isPresent() was found the least problematic. >> So I would like to get further with this proposal using the last >> webrev.04 version of the patch that uses ThreadLocal.isPresent() >> internally - still package-private at the moment. >> >> Here's the webrev (unchanged from the day it was published): >> >> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/ >> >> Would this version be OK? > I think looks quite good. > > One small nit is that the update to ThreadLocal.setInitialValue makes > it look like all locals are registered when setting the initial value. > What would you think about moving the instanceof check from > TerminatingThreadLocal.register so that it's a bit more obvious. > > -Alan Like the following? http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.05/ Do we need a test which proves that cached direct buffers are released when thread exits or would a unit test for TerminatingThreadLocal be enough? Maybe both? Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Hey Peter, I had written a test to test my version of the exit hooks. I can easily rework it to work with yours. Interested? To check that the native buffers are reclaimed promptly, we should be able to amend the test that tests the setting of the max size for the cached buffers (i.e., check that, after all the threads have exited, the total native count / size is 0). Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 20, 2018 at 10:08:48 AM, Peter Levart (peter.lev...@gmail.com) wrote: On 06/18/2018 05:41 PM, Alan Bateman wrote: > > > On 17/06/2018 22:20, Peter Levart wrote: >> Update: the discussion on concurrent-interest about possible future >> addition of public ThreadLocal.getIfPresent() or >> ThreadLocal.isPresent() has died out, but it nevertheless reached a >> point where ThreadLocal.isPresent() was found the least problematic. >> So I would like to get further with this proposal using the last >> webrev.04 version of the patch that uses ThreadLocal.isPresent() >> internally - still package-private at the moment. >> >> Here's the webrev (unchanged from the day it was published): >> >> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/ >> >> Would this version be OK? > I think looks quite good. > > One small nit is that the update to ThreadLocal.setInitialValue makes > it look like all locals are registered when setting the initial value. > What would you think about moving the instanceof check from > TerminatingThreadLocal.register so that it's a bit more obvious. > > -Alan Like the following? http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.05/ Do we need a test which proves that cached direct buffers are released when thread exits or would a unit test for TerminatingThreadLocal be enough? Maybe both? Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Good idea, as long as it re-uses the existing ThreadLocal infrastructure and doesn’t introduce an extra per-thread map. Making it a ThreadLocal subclass would be an excellent start. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 19, 2018 at 9:07:07 AM, David Lloyd (david.ll...@redhat.com) wrote: It may be confusing (to some, I guess) but it is consistent: the ThreadLocal subclass author has absolute control over how the value is presented to the user. Adding compute() or many of the other suggested variants will break that guarantee, which seems like kind of a big deal to me. What about introducing a different thread local API that has more modern behavior? Maybe a new subclass of ThreadLocal? On Mon, Jun 18, 2018 at 5:28 PM Martin Buchholz wrote: > > yes, the proposed new methods would use nulls differently. The original get() behavior of creating a mapping was a mistake, inconsistent with Map. Yes, it will cause confusion. But it's already confusing. > > On Mon, Jun 18, 2018 at 1:45 PM, David Lloyd wrote: >> >> On Mon, Jun 18, 2018 at 12:53 PM, Martin Buchholz wrote: >> > On Mon, Jun 18, 2018 at 10:21 AM, Tony Printezis < tprinte...@twitter.com> >> > wrote: >> > >> >> Martin, >> >> >> >> You are forgiven. :-) >> >> >> >> So, the (valid, I think) issue with getIfPresent(), as originally proposed >> >> by Peter, was that if get() was overridden in the subclass to somehow >> >> transform the returned value, getIfPresent() wouldn’t apply the same >> >> transformation. >> >> >> >> Doesn’t your compute() method have essentially the same issue? Apart from >> >> that, I personally like this proposal as I agree: one look-up is always >> >> better than two. >> >> >> >> >> > A non-prototype implementation would delegate compute into ThreadLocalMap >> > itself, where there is no risk of overriding. >> >> I don't think overriding is the risk; the risk would be compute* >> behaving inconsistently with regards to an overridden get*. >> >> >> -- >> - DML > > -- - DML
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Martin,
You are forgiven. :-)
So, the (valid, I think) issue with getIfPresent(), as originally proposed
by Peter, was that if get() was overridden in the subclass to somehow
transform the returned value, getIfPresent() wouldn’t apply the same
transformation.
Doesn’t your compute() method have essentially the same issue? Apart from
that, I personally like this proposal as I agree: one look-up is always
better than two.
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On June 18, 2018 at 10:13:02 AM, Martin Buchholz (marti...@google.com)
wrote:
I'm ignoring the direct buffers problem (sorry Tony) and wearing my
ReentrantReadWriteLock hat. I still like the idea of adding
ThreadLocal.compute(Function remappingFunction) and perhaps other such
map-inspired methods. RRWL wouldn't benefit much, since it already tries to
minimize use of ThreadLocal, but it would at least allow get() and remove()
to be coalesced on read-unlock.
RRWL never changes from one non-null value to another, it only switches
between absent and present. I'd like to avoid the two lookups due to get
and remove. Here's a prototype that does not yet help RRWL, but helps
callers switching between non-null values, and could probably be extended
via surgery to ThreadLocalMap:
public T compute(
java.util.function.Function
remappingFunction) {
final Thread currentThread = Thread.currentThread();
final ThreadLocalMap map = getMap(currentThread);
final ThreadLocalMap.Entry e = (map == null)
? null
: map.getEntry(this);
final T oldValue = (e == null) ? null : (T)e.value;
final T newValue = remappingFunction.apply(oldValue);
if (newValue == null) {
if (oldValue != null) {
map.remove(this);
}
} else if (e != null) {
e.value = newValue;
} else if (map != null) {
map.set(this, newValue);
} else {
createMap(currentThread, newValue);
}
return newValue;
}
On Sun, Jun 17, 2018 at 2:20 PM, Peter Levart
wrote:
> Update: the discussion on concurrent-interest about possible future
> addition of public ThreadLocal.getIfPresent() or ThreadLocal.isPresent()
> has died out, but it nevertheless reached a point where
> ThreadLocal.isPresent() was found the least problematic. So I would like to
> get further with this proposal using the last webrev.04 version of the
> patch that uses ThreadLocal.isPresent() internally - still package-private
> at the moment.
>
> Here's the webrev (unchanged from the day it was published):
>
> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/
>
> Would this version be OK?
>
> Regards, Peter
>
>
>
> On 06/06/18 20:55, Peter Levart wrote:
>
>
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Peter,
Inline.
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On June 7, 2018 at 5:21:43 AM, Peter Levart (peter.lev...@gmail.com) wrote:
Hi Tony,
Thanks for taking a look. Just a couple of comments inline...
On 06/06/18 22:38, Tony Printezis wrote:
- instead of overriding initialValue(), ThreadLocal.setInitialValue() calls
TerminatingThreadLocal.register() at the right moment
Thanks. I much prefer not having to introduce calculateInitialValue().
One extra suggestion: Given you introduced a call to TerminatingThreadLocal
from ThreadLocal, would it make sense to maybe do the same for set() and
remove() (you’d have to add an appropriate check in unregister) and not
override them in TerminatingTreadLocal? I totally get why you might not
want to do that (an extra check when a ThreadLocal is not the Terminating
one). I’m OK either way.
Yes, precisely. My 1st version did just that, but since there are usages of
ThreadLocal out there that are very performance sensitive, I opted for an
approach where there is zero performance regression for
non-TerminatingThreadLocal(s) when calling set() or remove(). A call-back
from setInitialValue is not so problematic as it usually occurs just once
per thread, but I can imagine a scenario where calls to get() and set()
occur very rapidly.
Yeah, I agree that this is a good trade-off given how the code is currently
structured.
An alternative to "T getIfPresent()" is a "boolean isPresent()" method.
Even if it remains package-private, with such method TerminatingThreadLocal
could be implemented as:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/
If TreadLocal.isPresent() was made public, the code could be further
simplified.
Something like:
if (tl.isPresent()) {
T val = t.get();
….
}
will do two look-ups if the value exists. However, that’s better than
allocating unnecessarily. So, I’ll take isPresent() over not having a way
to check whether a value exists.
Right, and in our scenario, it is just isPresent() that is called for every
termination of every thread (REGISTRY.isPresent()). .get() is then called
only when there's actually something to do.
Yes. I’m not worried about that at all.
Thumbs up from me.
Let's just wait for a day or two to see whether the discussion on
concurrency-interest gives us any additional ideas. Currently I'm thinking
of proposing the addition of isPresent() API. As far as this RFR is
concerned, I'm consequently promoting the latest webrev.04.
Again, #ThumbsUp from me. Thanks,
Tony
So any comments on that one? Alan?
Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Hi Peter,
Thanks for the updated webrev! Please see inline.
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On June 6, 2018 at 2:55:51 PM, Peter Levart (peter.lev...@gmail.com) wrote:
Ok, here's next webrev:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.03/
The changes from webrev.02 are:
- renamed JdkThreadLocal -> TerminatingThreadLocal
#ThumbsUp
- instead of overriding initialValue(), ThreadLocal.setInitialValue() calls
TerminatingThreadLocal.register() at the right moment
Thanks. I much prefer not having to introduce calculateInitialValue().
One extra suggestion: Given you introduced a call to TerminatingThreadLocal
from ThreadLocal, would it make sense to maybe do the same for set() and
remove() (you’d have to add an appropriate check in unregister) and not
override them in TerminatingTreadLocal? I totally get why you might not
want to do that (an extra check when a ThreadLocal is not the Terminating
one). I’m OK either way.
- TerminatingThreadLocal.threadTerminated() now takes a (T value) parameter
#ThumbsUp
- TerminatingThreadLocal.REGISTRY uses IdentityHashSet instead of HashSet
(if .equals()/.hashCode() are overriden in some TerminatingThreadLocal
subclass)
#ThumbsUp
David Lloyd has commented on concurrency-interest about
ThreadLocal.getIfPresent(). There is a concern that such new method would
be inconsistent with what ThreadLocal.get() returns from existing
ThreadLocal subclasses that override .get() and possibly transform the
value returned from super.get() on the way.
That’s a very good point.
An alternative to "T getIfPresent()" is a "boolean isPresent()" method.
Even if it remains package-private, with such method TerminatingThreadLocal
could be implemented as:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.04/
If TreadLocal.isPresent() was made public, the code could be further
simplified.
Something like:
if (tl.isPresent()) {
T val = t.get();
….
}
will do two look-ups if the value exists. However, that’s better than
allocating unnecessarily. So, I’ll take isPresent() over not having a way
to check whether a value exists.
Thumbs up from me.
Tony
Regards, Peter
On 06/06/18 18:58, Peter Levart wrote:
On 06/06/18 17:41, Tony Printezis wrote:
Peter,
You’re totally right re: JdkThreadLocal::initialValue being final and
cannot be overridden (I had completely missed the final keyword when I
first looked at the webrev). But it’d be nice to have the same API in both
versions of ThreadLocal. I assume you didn’t want to override get() since
you only wanted to update the REGISTRY the first time get() is called by a
thread. If getIfPresent() is exposed, would something like the following
work?
@Override
public T get() {
final T value = getIfPresent();
if (value != null) {
return value;
}
REGISTRY.get().add(this);
return super.get();
}
This would work, but if mapped value was 'null', it would keep calling
REGISTRY.get().add(this) for each get(). Logically correct, but with
useless overhead.
Let me try to do something that might satisfy you... (in next webrev).
One more question re: getIfPresent() (and maybe I’m overthinking this): It
returns null to indicate that a value is not present. Isn’t null a valid
ThreadLocal value? Would using an Optional here be more appropriate?
The problem with Optional is that it does not provide an additional value.
Optional.empty() is a replacement for null. There's no Optional.of(null).
So what would getIfPresent() return when there was a mapping present, but
that mapping was null?
Regards, Peter
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On June 6, 2018 at 11:12:44 AM, Peter Levart (peter.lev...@gmail.com) wrote:
Hi Tony, Alan,
On 06/06/2018 04:37 PM, Tony Printezis wrote:
Alan,
Thanks for your thoughts!
Peter,
Any chance of taking the two suggestions I made in an earlier e-mail into
account?
Right, I'll prepare new webrev with that shortly.
a) It’d be nice if users can override initialValue(), like when using the
standard ThreadLocal class, instead of calculateInitialValue(). (I can’t
come up with a clean solution on how to do that, though. I’ll think about
it for a bit.)
Your concern was that users might accidentally override initialValue()
instead of calculateInitialValue(), if I understood you correctly. If that
was the concern, they can't, because it is final. If the concern was that
users would want to override initialValue() because they are used to do so,
then perhaps a javadoc on final initialiValue() pointing to
calculateInitialValue() might help them. Do you agree? This is currently
internal API and consistent naming is not a big concern.
b) It’d be very helpful to pass T value to threadTerminated(), as I cannot
imagine many use-cases where the value will not be needed.
Agree. Will incl
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Peter,
You’re totally right re: JdkThreadLocal::initialValue being final and
cannot be overridden (I had completely missed the final keyword when I
first looked at the webrev). But it’d be nice to have the same API in both
versions of ThreadLocal. I assume you didn’t want to override get() since
you only wanted to update the REGISTRY the first time get() is called by a
thread. If getIfPresent() is exposed, would something like the following
work?
@Override
public T get() {
final T value = getIfPresent();
if (value != null) {
return value;
}
REGISTRY.get().add(this);
return super.get();
}
One more question re: getIfPresent() (and maybe I’m overthinking this): It
returns null to indicate that a value is not present. Isn’t null a valid
ThreadLocal value? Would using an Optional here be more appropriate?
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On June 6, 2018 at 11:12:44 AM, Peter Levart (peter.lev...@gmail.com) wrote:
Hi Tony, Alan,
On 06/06/2018 04:37 PM, Tony Printezis wrote:
Alan,
Thanks for your thoughts!
Peter,
Any chance of taking the two suggestions I made in an earlier e-mail into
account?
Right, I'll prepare new webrev with that shortly.
a) It’d be nice if users can override initialValue(), like when using the
standard ThreadLocal class, instead of calculateInitialValue(). (I can’t
come up with a clean solution on how to do that, though. I’ll think about
it for a bit.)
Your concern was that users might accidentally override initialValue()
instead of calculateInitialValue(), if I understood you correctly. If that
was the concern, they can't, because it is final. If the concern was that
users would want to override initialValue() because they are used to do so,
then perhaps a javadoc on final initialiValue() pointing to
calculateInitialValue() might help them. Do you agree? This is currently
internal API and consistent naming is not a big concern.
b) It’d be very helpful to pass T value to threadTerminated(), as I cannot
imagine many use-cases where the value will not be needed.
Agree. Will include that in new webrev.
Re: renaming JdkThreadLocal: ThreadLocalWithExitHooks?
Hm. Exit Hooks are already something that is used in JVM (Threads started
when VM is about to exit), so this might be confusing for someone.
- DisposableThreadLocal
- ThreadLocalWithCleanup
And my favorite:
- TerminatingThreadLocal
Re: exposing getIfPresent() : Yes! Pretty please! :-) This will be very
helpful and can avoid completely unnecessary allocations.
I agree that this would be generally useful functionality. Might be good to
ask for opinion on concurrency-interest mailing list first. Will do that.
Regards, Peter
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On June 6, 2018 at 9:38:05 AM, Alan Bateman (alan.bate...@oracle.com) wrote:
On 30/05/2018 22:16, Peter Levart wrote:
> I thought there would be some hint from Alan about which of the two
> paths we should take for more refinement.
>
> The Tony's:
>
> http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/
>
> Or the Alan's with my mods:
>
> http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/
>
Finally getting back to this one.
I don't think the two approaches are all that different now. Tony's
point on the number of hooks vs. number of locals was an important point
but with Peter's update to use a thread local registry then I think we
have easy to maintain solution in the DBBCache_Cleanup/webrev.02 patch.
So I think I prefer that approach. We need to better name for
"JdkThreadLocal", something to indicate that it holds resources or it
notified when a thread terminates.
Also along the way, we touched on exposing getIfPresent and we should
look at that again. If it is expose then it fits well with the second
approach too.
-Alan
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Alan, Thanks for your thoughts! Peter, Any chance of taking the two suggestions I made in an earlier e-mail into account? a) It’d be nice if users can override initialValue(), like when using the standard ThreadLocal class, instead of calculateInitialValue(). (I can’t come up with a clean solution on how to do that, though. I’ll think about it for a bit.) b) It’d be very helpful to pass T value to threadTerminated(), as I cannot imagine many use-cases where the value will not be needed. Re: renaming JdkThreadLocal: ThreadLocalWithExitHooks? Re: exposing getIfPresent() : Yes! Pretty please! :-) This will be very helpful and can avoid completely unnecessary allocations. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On June 6, 2018 at 9:38:05 AM, Alan Bateman (alan.bate...@oracle.com) wrote: On 30/05/2018 22:16, Peter Levart wrote: > I thought there would be some hint from Alan about which of the two > paths we should take for more refinement. > > The Tony's: > > http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/ > > Or the Alan's with my mods: > > http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/ > Finally getting back to this one. I don't think the two approaches are all that different now. Tony's point on the number of hooks vs. number of locals was an important point but with Peter's update to use a thread local registry then I think we have easy to maintain solution in the DBBCache_Cleanup/webrev.02 patch. So I think I prefer that approach. We need to better name for "JdkThreadLocal", something to indicate that it holds resources or it notified when a thread terminates. Also along the way, we touched on exposing getIfPresent and we should look at that again. If it is expose then it fits well with the second approach too. -Alan
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Hey Alan,
Any thoughts on this? (with apologies for the ping)
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 30, 2018 at 5:16:44 PM, Peter Levart (peter.lev...@gmail.com) wrote:
I thought there would be some hint from Alan about which of the two paths
we should take for more refinement.
The Tony's:
http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/
Or the Alan's with my mods:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/
Regards, Peter
On 05/30/18 22:46, Tony Printezis wrote:
Hi all,
Any more thoughts on this? (with apologies for the ping)
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 18, 2018 at 3:58:57 PM, Tony Printezis (tprinte...@twitter.com)
wrote:
Hi again,
Stylistically, I strongly prefer this version over the previous one (the
one with the doubly-linked list of JdkThreadLocal entries) you had posted.
This one is a lot cleaner.
A few suggestions:
* I’m not crazy about the fact that the users have to override
calculateInitialValue() instead of initialValue() as it will be a bit
error-prone. If they accidentally override initialValue() then the
per-thread registering is not going to happen. Maybe I’m overthinking this.
One way to get round this is for each JdkThreadLocal instance to delegate
calls to get(), set(), and remove() to a private ThreadLocal instance,
which can in turn delegate initialValue() to the outer instance. (Hope this
makes sense?) I did a quick prototype of this and it seems to work, but I
didn’t heavily test it.
* I would prefer if the method users override actually took the
thread-local value as a parameter, i.e., protected void threadTerminated(T
value). This is very easy to add:
protected void threadTerminated(T value) {
}
private void threadTerminated() {
threadTerminated(get());
}
and I think it will be easier to use, as most uses will probably need to do
get() anyway.
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 18, 2018 at 4:23:19 AM, Peter Levart (peter.lev...@gmail.com) wrote:
It's good to have alternative implementations on the table. Here's another
variant that is space neutral for threads that don't use JdkThreadLocal,
but also scales to many JdkThreadLocal instances:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/
Now we only need an arbiter to decide which one! :-)
Regards, Peter
On 05/17/18 22:39, Peter Levart wrote:
Hi Tony,
If we anticipate only small number of JdkThreadLocal(s) (there will be only
one for the start) then this is a plausible solution. Then perhaps this
limitation should be written somewhere in the JdkThreadLocal javadoc so
that one day somebody will not be tempted to use JdkThreadLocal(s) en
masse. Let's say there will be a few more JdkThreadLocal(s) in the future.
Are we willing to pay for a few lookups into a ThreadLocalMap at each and
every thread's exit even though such thread did not register a mapping for
any JdkThreadLocal? Is an additional reference field in each and every
ThreadLocalMap (one per Thread that uses thread locals) a bigger price to
pay? I don't know. Will let others comment on this.
Otherwise the code looks good. Just a couple of observations:
Since private static method JdkThreadLocal.add is only called from
JdkThreadLocal constructor with just constructed instance (this), there's
no possibility for it to be called twice or more times with the same
instance. The check for duplicates could go away then, right?
You keep an array of Entry objects which are just wrappers for
JdkThreadLocal objects. Are you already planning for Entry to become a
WeakReference? Otherwise you could just keep JdkThreadLocal objects in the
array directly.
Regards, Peter
On 05/17/18 20:25, Tony Printezis wrote:
Hi all,
I have a new version of the code for your consideration:
http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/
I basically combined our two approaches. The usage is as Alan had proposed
it: Users have to use JdkThreadLocal (which is only available within
java.base) and override threadTerminated(). However, I keep track of
JdkThreadLocal instances globally (as I did before) and not per-thread.
This way we don’t need to add any unnecessary complexity to ThreadLocalMap.
Currently, I don’t allow entries to be purged if the JdkThreadLocal
instance becomes otherwise unreachable. I can easily add that functionality
if needed (I can use WeakReferences for that). However, for the uses we’re
considering, is it really necessary?
Thoughts?
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprinte...@twitter.com)
wrote:
Peter,
In my proposal, you can register the exit hook in the ThreadLocal c’tor, so
it’s almost as nice as Alan’s in that respect (and it doesn't require an
extra field per ThreadLocal plus two extra fields per JdkEntry)
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Hi all,
Any more thoughts on this? (with apologies for the ping)
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 18, 2018 at 3:58:57 PM, Tony Printezis (tprinte...@twitter.com)
wrote:
Hi again,
Stylistically, I strongly prefer this version over the previous one (the
one with the doubly-linked list of JdkThreadLocal entries) you had posted.
This one is a lot cleaner.
A few suggestions:
* I’m not crazy about the fact that the users have to override
calculateInitialValue() instead of initialValue() as it will be a bit
error-prone. If they accidentally override initialValue() then the
per-thread registering is not going to happen. Maybe I’m overthinking this.
One way to get round this is for each JdkThreadLocal instance to delegate
calls to get(), set(), and remove() to a private ThreadLocal instance,
which can in turn delegate initialValue() to the outer instance. (Hope this
makes sense?) I did a quick prototype of this and it seems to work, but I
didn’t heavily test it.
* I would prefer if the method users override actually took the
thread-local value as a parameter, i.e., protected void threadTerminated(T
value). This is very easy to add:
protected void threadTerminated(T value) {
}
private void threadTerminated() {
threadTerminated(get());
}
and I think it will be easier to use, as most uses will probably need to do
get() anyway.
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 18, 2018 at 4:23:19 AM, Peter Levart (peter.lev...@gmail.com) wrote:
It's good to have alternative implementations on the table. Here's another
variant that is space neutral for threads that don't use JdkThreadLocal,
but also scales to many JdkThreadLocal instances:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/
Now we only need an arbiter to decide which one! :-)
Regards, Peter
On 05/17/18 22:39, Peter Levart wrote:
Hi Tony,
If we anticipate only small number of JdkThreadLocal(s) (there will be only
one for the start) then this is a plausible solution. Then perhaps this
limitation should be written somewhere in the JdkThreadLocal javadoc so
that one day somebody will not be tempted to use JdkThreadLocal(s) en
masse. Let's say there will be a few more JdkThreadLocal(s) in the future.
Are we willing to pay for a few lookups into a ThreadLocalMap at each and
every thread's exit even though such thread did not register a mapping for
any JdkThreadLocal? Is an additional reference field in each and every
ThreadLocalMap (one per Thread that uses thread locals) a bigger price to
pay? I don't know. Will let others comment on this.
Otherwise the code looks good. Just a couple of observations:
Since private static method JdkThreadLocal.add is only called from
JdkThreadLocal constructor with just constructed instance (this), there's
no possibility for it to be called twice or more times with the same
instance. The check for duplicates could go away then, right?
You keep an array of Entry objects which are just wrappers for
JdkThreadLocal objects. Are you already planning for Entry to become a
WeakReference? Otherwise you could just keep JdkThreadLocal objects in the
array directly.
Regards, Peter
On 05/17/18 20:25, Tony Printezis wrote:
Hi all,
I have a new version of the code for your consideration:
http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/
I basically combined our two approaches. The usage is as Alan had proposed
it: Users have to use JdkThreadLocal (which is only available within
java.base) and override threadTerminated(). However, I keep track of
JdkThreadLocal instances globally (as I did before) and not per-thread.
This way we don’t need to add any unnecessary complexity to ThreadLocalMap.
Currently, I don’t allow entries to be purged if the JdkThreadLocal
instance becomes otherwise unreachable. I can easily add that functionality
if needed (I can use WeakReferences for that). However, for the uses we’re
considering, is it really necessary?
Thoughts?
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprinte...@twitter.com)
wrote:
Peter,
In my proposal, you can register the exit hook in the ThreadLocal c’tor, so
it’s almost as nice as Alan’s in that respect (and it doesn't require an
extra field per ThreadLocal plus two extra fields per JdkEntry). :-)
But, I do like the addition of the JdkEntry list to avoid unnecessarily
iterating over all the map entries (which was my main concern with Alan’s
original webrev). I’ll be totally happy with a version of this.
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.lev...@gmail.com) wrote:
Hi,
On 05/11/18 16:13, Alan Bateman wrote:
On 08/05/2018 16:07, Tony Printezis wrote:
Hi all,
Following the discussion on this a few weeks ago, here’s the first version
of the change:
h
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Hi again,
Stylistically, I strongly prefer this version over the previous one (the
one with the doubly-linked list of JdkThreadLocal entries) you had posted.
This one is a lot cleaner.
A few suggestions:
* I’m not crazy about the fact that the users have to override
calculateInitialValue() instead of initialValue() as it will be a bit
error-prone. If they accidentally override initialValue() then the
per-thread registering is not going to happen. Maybe I’m overthinking this.
One way to get round this is for each JdkThreadLocal instance to delegate
calls to get(), set(), and remove() to a private ThreadLocal instance,
which can in turn delegate initialValue() to the outer instance. (Hope this
makes sense?) I did a quick prototype of this and it seems to work, but I
didn’t heavily test it.
* I would prefer if the method users override actually took the
thread-local value as a parameter, i.e., protected void threadTerminated(T
value). This is very easy to add:
protected void threadTerminated(T value) {
}
private void threadTerminated() {
threadTerminated(get());
}
and I think it will be easier to use, as most uses will probably need to do
get() anyway.
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 18, 2018 at 4:23:19 AM, Peter Levart (peter.lev...@gmail.com) wrote:
It's good to have alternative implementations on the table. Here's another
variant that is space neutral for threads that don't use JdkThreadLocal,
but also scales to many JdkThreadLocal instances:
http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.02/
Now we only need an arbiter to decide which one! :-)
Regards, Peter
On 05/17/18 22:39, Peter Levart wrote:
Hi Tony,
If we anticipate only small number of JdkThreadLocal(s) (there will be only
one for the start) then this is a plausible solution. Then perhaps this
limitation should be written somewhere in the JdkThreadLocal javadoc so
that one day somebody will not be tempted to use JdkThreadLocal(s) en
masse. Let's say there will be a few more JdkThreadLocal(s) in the future.
Are we willing to pay for a few lookups into a ThreadLocalMap at each and
every thread's exit even though such thread did not register a mapping for
any JdkThreadLocal? Is an additional reference field in each and every
ThreadLocalMap (one per Thread that uses thread locals) a bigger price to
pay? I don't know. Will let others comment on this.
Otherwise the code looks good. Just a couple of observations:
Since private static method JdkThreadLocal.add is only called from
JdkThreadLocal constructor with just constructed instance (this), there's
no possibility for it to be called twice or more times with the same
instance. The check for duplicates could go away then, right?
You keep an array of Entry objects which are just wrappers for
JdkThreadLocal objects. Are you already planning for Entry to become a
WeakReference? Otherwise you could just keep JdkThreadLocal objects in the
array directly.
Regards, Peter
On 05/17/18 20:25, Tony Printezis wrote:
Hi all,
I have a new version of the code for your consideration:
http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/
I basically combined our two approaches. The usage is as Alan had proposed
it: Users have to use JdkThreadLocal (which is only available within
java.base) and override threadTerminated(). However, I keep track of
JdkThreadLocal instances globally (as I did before) and not per-thread.
This way we don’t need to add any unnecessary complexity to ThreadLocalMap.
Currently, I don’t allow entries to be purged if the JdkThreadLocal
instance becomes otherwise unreachable. I can easily add that functionality
if needed (I can use WeakReferences for that). However, for the uses we’re
considering, is it really necessary?
Thoughts?
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprinte...@twitter.com)
wrote:
Peter,
In my proposal, you can register the exit hook in the ThreadLocal c’tor, so
it’s almost as nice as Alan’s in that respect (and it doesn't require an
extra field per ThreadLocal plus two extra fields per JdkEntry). :-)
But, I do like the addition of the JdkEntry list to avoid unnecessarily
iterating over all the map entries (which was my main concern with Alan’s
original webrev). I’ll be totally happy with a version of this.
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.lev...@gmail.com) wrote:
Hi,
On 05/11/18 16:13, Alan Bateman wrote:
On 08/05/2018 16:07, Tony Printezis wrote:
Hi all,
Following the discussion on this a few weeks ago, here’s the first version
of the change:
http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/
I think the consensus was that it’d be easier if the exit hooks were only
available within java.base. Is it enough that I added the functionality to
the jdk.internal.mis
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Hi Peter, Thanks for taking a look at the new webrev! Initially, I think we’re expecting two uses of JdkThreadLocal: one in sun.nio.ch.Utils, shown on my webrev and my original motivation for working on this, and one in sun.nio.fs.NativeBuffers, as shown on Alan’s webrev (I’m not familiar with that part of the code at all; I assume it’s addressing a similar issue to sun.nio.ch.Utils). When I originally brought up this issue, Alan said that he's only expecting 2-3 uses (literally) inside java.base, so I did the implementation accordingly and tried to keep it as simple as possible. We could maybe look at other uses of ThreadLocal inside java.base to get a better sense of how many more will benefit from a thread termination callback? Response to your comments: I can definitely add javadoc to explain the limitations of the implementation. It takes me a long time to write coherent javadoc / comments, so I wanted to make sure we have the right approach first before I did that. :-) Re: sanity tests in add(): I was being paranoid but, sure, I can remove them. Re: Entry objects: You’re absolutely right. I did it this way to make it easier to extend WeakReference if needed. It also keeps the code a bit less verbose. I can use JdkThreadLocal directly. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 17, 2018 at 4:39:20 PM, Peter Levart (peter.lev...@gmail.com) wrote: Hi Tony, If we anticipate only small number of JdkThreadLocal(s) (there will be only one for the start) then this is a plausible solution. Then perhaps this limitation should be written somewhere in the JdkThreadLocal javadoc so that one day somebody will not be tempted to use JdkThreadLocal(s) en masse. Let's say there will be a few more JdkThreadLocal(s) in the future. Are we willing to pay for a few lookups into a ThreadLocalMap at each and every thread's exit even though such thread did not register a mapping for any JdkThreadLocal? Is an additional reference field in each and every ThreadLocalMap (one per Thread that uses thread locals) a bigger price to pay? I don't know. Will let others comment on this. Otherwise the code looks good. Just a couple of observations: Since private static method JdkThreadLocal.add is only called from JdkThreadLocal constructor with just constructed instance (this), there's no possibility for it to be called twice or more times with the same instance. The check for duplicates could go away then, right? You keep an array of Entry objects which are just wrappers for JdkThreadLocal objects. Are you already planning for Entry to become a WeakReference? Otherwise you could just keep JdkThreadLocal objects in the array directly. Regards, Peter On 05/17/18 20:25, Tony Printezis wrote: Hi all, I have a new version of the code for your consideration: http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/ I basically combined our two approaches. The usage is as Alan had proposed it: Users have to use JdkThreadLocal (which is only available within java.base) and override threadTerminated(). However, I keep track of JdkThreadLocal instances globally (as I did before) and not per-thread. This way we don’t need to add any unnecessary complexity to ThreadLocalMap. Currently, I don’t allow entries to be purged if the JdkThreadLocal instance becomes otherwise unreachable. I can easily add that functionality if needed (I can use WeakReferences for that). However, for the uses we’re considering, is it really necessary? Thoughts? Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprinte...@twitter.com) wrote: Peter, In my proposal, you can register the exit hook in the ThreadLocal c’tor, so it’s almost as nice as Alan’s in that respect (and it doesn't require an extra field per ThreadLocal plus two extra fields per JdkEntry). :-) But, I do like the addition of the JdkEntry list to avoid unnecessarily iterating over all the map entries (which was my main concern with Alan’s original webrev). I’ll be totally happy with a version of this. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.lev...@gmail.com) wrote: Hi, On 05/11/18 16:13, Alan Bateman wrote: On 08/05/2018 16:07, Tony Printezis wrote: Hi all, Following the discussion on this a few weeks ago, here’s the first version of the change: http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/ I think the consensus was that it’d be easier if the exit hooks were only available within java.base. Is it enough that I added the functionality to the jdk.internal.misc package? (And is jdk.internal.misc the best place for this?) I’ll also add a test for the new functionality. But let’s first come up with an approach that everyone is happy with. :-) Peter's approach in early April was clean (and we should come to the getIfPresent discussion) but it adds a field to
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Hi all, I have a new version of the code for your consideration: http://cr.openjdk.java.net/~tonyp/8202788/webrev.1/ I basically combined our two approaches. The usage is as Alan had proposed it: Users have to use JdkThreadLocal (which is only available within java.base) and override threadTerminated(). However, I keep track of JdkThreadLocal instances globally (as I did before) and not per-thread. This way we don’t need to add any unnecessary complexity to ThreadLocalMap. Currently, I don’t allow entries to be purged if the JdkThreadLocal instance becomes otherwise unreachable. I can easily add that functionality if needed (I can use WeakReferences for that). However, for the uses we’re considering, is it really necessary? Thoughts? Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 14, 2018 at 12:40:28 PM, Tony Printezis (tprinte...@twitter.com) wrote: Peter, In my proposal, you can register the exit hook in the ThreadLocal c’tor, so it’s almost as nice as Alan’s in that respect (and it doesn't require an extra field per ThreadLocal plus two extra fields per JdkEntry). :-) But, I do like the addition of the JdkEntry list to avoid unnecessarily iterating over all the map entries (which was my main concern with Alan’s original webrev). I’ll be totally happy with a version of this. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.lev...@gmail.com) wrote: Hi, On 05/11/18 16:13, Alan Bateman wrote: On 08/05/2018 16:07, Tony Printezis wrote: Hi all, Following the discussion on this a few weeks ago, here’s the first version of the change: http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/ I think the consensus was that it’d be easier if the exit hooks were only available within java.base. Is it enough that I added the functionality to the jdk.internal.misc package? (And is jdk.internal.misc the best place for this?) I’ll also add a test for the new functionality. But let’s first come up with an approach that everyone is happy with. :-) Peter's approach in early April was clean (and we should come to the getIfPresent discussion) but it adds a field to Thread for the callback list. If I read your approach correctly, you are avoiding that by maintaining an array of hooks in ThreadLocalExitHooks. Another approach to try is a java.base-internal ThreadLocal that defines a method to be invoked when a thread terminates. Something like the following: http://cr.openjdk.java.net/~alanb/8202788/webrev/index.html -Alan >From the API perspective, Alan's suggestion is the most attractive one as it puts the method to where it belongs - into the ThreadLocal instance. But the implementation could be improved a bit. I don't like the necessity to iterate over all ThreadLocal(s) to filter out JdkThreadLocal(s). There might be a situation where there's plenty of ThreadLocal(s) registered per exiting thread which would produce a spike in CPU processing at thread exit. The way to avoid this would be to maintain a separate linked list of entries that contains just those with JdkThreadLocal(s). Like in this modification of Alan's patch, for example: http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.01/ (Only ThreadLocal class is modified from Alan's patch) What do you think? Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Peter, In my proposal, you can register the exit hook in the ThreadLocal c’tor, so it’s almost as nice as Alan’s in that respect (and it doesn't require an extra field per ThreadLocal plus two extra fields per JdkEntry). :-) But, I do like the addition of the JdkEntry list to avoid unnecessarily iterating over all the map entries (which was my main concern with Alan’s original webrev). I’ll be totally happy with a version of this. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 12, 2018 at 6:44:08 AM, Peter Levart (peter.lev...@gmail.com) wrote: Hi, On 05/11/18 16:13, Alan Bateman wrote: On 08/05/2018 16:07, Tony Printezis wrote: Hi all, Following the discussion on this a few weeks ago, here’s the first version of the change: http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/ I think the consensus was that it’d be easier if the exit hooks were only available within java.base. Is it enough that I added the functionality to the jdk.internal.misc package? (And is jdk.internal.misc the best place for this?) I’ll also add a test for the new functionality. But let’s first come up with an approach that everyone is happy with. :-) Peter's approach in early April was clean (and we should come to the getIfPresent discussion) but it adds a field to Thread for the callback list. If I read your approach correctly, you are avoiding that by maintaining an array of hooks in ThreadLocalExitHooks. Another approach to try is a java.base-internal ThreadLocal that defines a method to be invoked when a thread terminates. Something like the following: http://cr.openjdk.java.net/~alanb/8202788/webrev/index.html -Alan >From the API perspective, Alan's suggestion is the most attractive one as it puts the method to where it belongs - into the ThreadLocal instance. But the implementation could be improved a bit. I don't like the necessity to iterate over all ThreadLocal(s) to filter out JdkThreadLocal(s). There might be a situation where there's plenty of ThreadLocal(s) registered per exiting thread which would produce a spike in CPU processing at thread exit. The way to avoid this would be to maintain a separate linked list of entries that contains just those with JdkThreadLocal(s). Like in this modification of Alan's patch, for example: http://cr.openjdk.java.net/~plevart/jdk-dev/DBBCache_Cleanup/webrev.01/ (Only ThreadLocal class is modified from Alan's patch) What do you think? Regards, Peter
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Alan and Peter, First, I need to apologize: I completely missed Peter’s proposal (for some reason Peter’s follow-up e-mails show up blank on my mail client). Could someone point me to it so I can take a look? Re: having info per thread vs. globally: Having a couple more objects and a field per-thread will probably not be a huge deal, memory footprint-wise. But, and with my GC implementer hat on :-), avoiding that and only maintaining a single global entry per-ThreadLocal can only be a good thing, IMHO. Re: Alan’s approach vs. my approach: I like Alan’s approach given that it doesn’t require any additional data structures / memory footprint to keep track of the hooks (only an extra class). The only downside I can think of is that it requires an iteration over all entries in each ThreadLocal map even if there are no ThreadLocals with an exit hook. My proposal only iterates over the exit hooks (and it’s essentially a nop if there are none). If we assume there will be a very small number of exit hooks, and potentially many entries in some TL maps, this will be a win and keep Thread::exit a bit leaner. Having said all this, I’ll be very happy to just get this fix done. So, feel free to decide on which approach is most appropriate! :-) I’ll be happy with any of the proposals. Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 11, 2018 at 10:13:30 AM, Alan Bateman (alan.bate...@oracle.com) wrote: On 08/05/2018 16:07, Tony Printezis wrote: > Hi all, > > Following the discussion on this a few weeks ago, here’s the first version > of the change: > > http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/ > > I think the consensus was that it’d be easier if the exit hooks were only > available within java.base. Is it enough that I added the functionality to > the jdk.internal.misc package? (And is jdk.internal.misc the best place for > this?) > > I’ll also add a test for the new functionality. But let’s first come up > with an approach that everyone is happy with. :-) > Peter's approach in early April was clean (and we should come to the getIfPresent discussion) but it adds a field to Thread for the callback list. If I read your approach correctly, you are avoiding that by maintaining an array of hooks in ThreadLocalExitHooks. Another approach to try is a java.base-internal ThreadLocal that defines a method to be invoked when a thread terminates. Something like the following: http://cr.openjdk.java.net/~alanb/8202788/webrev/index.html -Alan
Re: RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
David, Please see inline. — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On May 8, 2018 at 11:44:11 AM, David Lloyd (david.ll...@redhat.com) wrote: I'm not a reviewer, but I would ask: how sure are we that it's OK to use lambdas from here? Is there a chance that the magic bootstrap stuff won't yet be initialized at the time when an early thread could exit for whatever reason? I’m not aware that lambdas cannot be used at particular points in the JVM’s execution. Is that really a concern? (FWIW, the code I posted compiles and runs fine.) Anyway I think using a lambda with forEach is probably overkill in atThreadExit. A simple for loop would suffice IMO. That’s not straightforward, though. Note that the iteration is done by the ThreadLocal class, not the ThreadLocalExitHooks class. So, a for-loop won’t work, unless I expose the array to the ThreadLocal class, which I’d rather not do. I could use an Iterator. But using forEach() is a lot nicer (IMHO of course). Also, I would be quite surprised if there wasn't a way to get a system property from system code without having to use doPrivileged; that might bear some researching. I just copied how the Utils class reads the jdk.nio.maxCachedBufferSize property (see the existing getMaxCachedBufferSize() method). Could this all be simplified to drop the necessity for passing around the thread local map and thread local instances? The sun.nio.ch.Util has static access to its ThreadLocal, so really all it needs is to be able to give a Runnable to run at thread exit and it can take care of the rest. Not quite sure what you mean by “passing around the thread local map and thread local instances”. The map is only passed from Thread to ThreadLocal, so the latter can call exit hooks (if any). So, the map doesn’t escape the Thread / ThreadLocal classes. The thread local instance is only used to associate that instance with an exit hook in the ThreadLocalExitHooks class. It’d be nice if I could pass a Runnable to be called at thread exit. However, how do I do that? Will there be a method on ThreadLocal to set this Runnable. If that’s the case, can we arrange for a method on ThreadLocal to only be used within java.base module (remember: we don’t want this functionality to be used outside java.base)? Additionally, if it’s just a Runnable, how does it get the thread’s ThreadLocal value when it runs? If the ThreadLocal is not set for that thread, get() will create it, which will be a waste of an allocation. It could be a simple as keeping an ArrayList of Runnables on Thread or in another ThreadLocal. But this requires at least a couple of objects per Thread. What I implemented has a single entry for each ThreadLocal, irrespective of how many threads set it (so a lot less overhead). Tony On Tue, May 8, 2018 at 10:07 AM, Tony Printezis wrote: > Hi all, > > Following the discussion on this a few weeks ago, here’s the first version > of the change: > > http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/ > > I think the consensus was that it’d be easier if the exit hooks were only > available within java.base. Is it enough that I added the functionality to > the jdk.internal.misc package? (And is jdk.internal.misc the best place for > this?) > > I’ll also add a test for the new functionality. But let’s first come up > with an approach that everyone is happy with. :-) > > One additional question: I put the new functionality conditional on a > property (jdk.nio.freeBuffersAtThreadExit) and it’s off by default. Should > I make it on by default? Or just not add the property all-together? > > Tony > > > — > Tony Printezis | @TonyPrintezis | tprinte...@twitter.com -- - DML
RFR: JDK-8202788: Explicitly reclaim cached thread-local direct buffers at thread exit
Hi all, Following the discussion on this a few weeks ago, here’s the first version of the change: http://cr.openjdk.java.net/~tonyp/8202788/webrev.0/ I think the consensus was that it’d be easier if the exit hooks were only available within java.base. Is it enough that I added the functionality to the jdk.internal.misc package? (And is jdk.internal.misc the best place for this?) I’ll also add a test for the new functionality. But let’s first come up with an approach that everyone is happy with. :-) One additional question: I put the new functionality conditional on a property (jdk.nio.freeBuffersAtThreadExit) and it’s off by default. Should I make it on by default? Or just not add the property all-together? Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
Re: Promptly freeing the per-thread cached direct buffers when a thread exits
Hi Alan, Ah, I hadn’t realized that there’s already some tight coupling between Thread and nio. OK, I’ll just call into sun.nio directly and see what the reviewers say. :-) Is there a CR for this already? Or should I create one? Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On April 6, 2018 at 12:45:29 PM, Alan Bateman (alan.bate...@oracle.com) wrote: On 06/04/2018 15:08, Tony Printezis wrote: Hi Alan, Calling sun.nio directly from java.lang seemed a bit dodgy to me, which is why I proposed some type of exit hook (maybe I overthought this?). But that’d be OK, could make this change a lot simpler. :-) And, yes, I came across the issue of not being able to query whether a ThreadLocal exists on a Thread when I implemented my prototype. Which is why I think introducing an exit hook on ThreadLocal, instead of Thread, is probably the better approach (it will only be called if the ThreadLocal exists). java.lang.Thread is already tightly connected to blocking I/O operations (Thread::interrupt for example). In any case, the concern with exposing exit hooks in the API is that it means running arbitrary code when exiting, something that would need a lot of consideration before going there. -Alan
Re: Promptly freeing the per-thread cached direct buffers when a thread exits
— Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On April 6, 2018 at 12:16:10 PM, David Lloyd (david.ll...@redhat.com) wrote: On Fri, Apr 6, 2018 at 8:57 AM, Tony Printezis wrote: >> ThreadLocal clearing > > Could you clarify what you mean by ThreadLocal clearing? I mean calling ThreadLocal#remove(). I see. So, anyone who subclasses ThreadLocal can override remove(). And if remove() is called by Thread::exit, it can be used as an exit hook. (David Holmes, if this is what you meant in your e-mail: apologies; I misunderstood.) > I like the suggestion to add an overridable exit() method to ThreadLocal. If > you want to avoid calling user code by Thread::exit, would adding > ThreadLocals (which are tagged appropriately) to a queue for later > processing a better approach (similar to the mechanism used for References / > ReferencesQueues)? The user can of course create a memory leak by not > polling the queue frequently enough. But, that’s also the case for > References. And at least user code cannot block Thread::exit. It's more complexity, and at some point you have to ask: is it better to block thread A or thread B? At least blocking thread A is somewhat expected. I agree re: it’d add complexity. #simplify :-) Tony -- - DML
Re: Promptly freeing the per-thread cached direct buffers when a thread exits
Was there a reason why this was not introduced in the first place?
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On April 6, 2018 at 8:49:17 AM, Peter Levart (peter.lev...@gmail.com) wrote:
On 04/06/2018 10:02 AM, Alan Bateman wrote:
> On 05/04/2018 22:45, Tony Printezis wrote:
>> Hi all,
>>
>> We recently hit another interesting issue with the NIO thread-local
>> DirectByteBuffer cache. One of our services seems to create and drop
>> threads at regular intervals. I assume this is due to a thread pool
>> dynamically resizing itself.
>>
>> Let's say a thread starts, lives long enough for its Thread object to be
>> promoted to the old gen (along with its cached direct buffer), then
>> exits.
>> This will result in its cached direct buffer(s) being unreachable in the
>> old gen and will only be reclaimed after the next full GC /
>> concurrent GC
>> cycle.
>>
> Right, if a short lived thread does I/O with a buffer backed by an
> array in the heap then any direct buffers in its thread local cache
> won't be freed until there is a GC and reference processing. It's
> something that I've prototyped a few times and always leaned towards
> not exposing anything in the API, instead just hooking into the thread
> exit to clear the buffer cache. One thing to watch out for is that the
> buffer cache may not exist (as the thread didn't do any I/O with heap
> buffers) so you'll end up creating (an empty) buffer cache at thread
> exit. That is benign of course but still unsettling to have additional
> code executing at this time.
>
> -Alan
An internal method, let's say ThreadLocal.probe(), that would return
thread-local value only if it has already been initialized, would be
helpfull. Maybe it could be exposed as new public API too. I remember
that I needed it in the past:
public T probe() {
Thread t = Thread.currentThread();
ThreadLocalMap map = getMap(t);
if (map != null) {
ThreadLocalMap.Entry e = map.getEntry(this);
if (e != null) {
@SuppressWarnings("unchecked")
T result = (T)e.value;
return result;
}
}
return null;
}
Regards, Peter
Re: Promptly freeing the per-thread cached direct buffers when a thread exits
Hi Alan, Calling sun.nio directly from java.lang seemed a bit dodgy to me, which is why I proposed some type of exit hook (maybe I overthought this?). But that’d be OK, could make this change a lot simpler. :-) And, yes, I came across the issue of not being able to query whether a ThreadLocal exists on a Thread when I implemented my prototype. Which is why I think introducing an exit hook on ThreadLocal, instead of Thread, is probably the better approach (it will only be called if the ThreadLocal exists). Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On April 6, 2018 at 4:02:56 AM, Alan Bateman (alan.bate...@oracle.com) wrote: On 05/04/2018 22:45, Tony Printezis wrote: > Hi all, > > We recently hit another interesting issue with the NIO thread-local > DirectByteBuffer cache. One of our services seems to create and drop > threads at regular intervals. I assume this is due to a thread pool > dynamically resizing itself. > > Let's say a thread starts, lives long enough for its Thread object to be > promoted to the old gen (along with its cached direct buffer), then exits. > This will result in its cached direct buffer(s) being unreachable in the > old gen and will only be reclaimed after the next full GC / concurrent GC > cycle. > Right, if a short lived thread does I/O with a buffer backed by an array in the heap then any direct buffers in its thread local cache won't be freed until there is a GC and reference processing. It's something that I've prototyped a few times and always leaned towards not exposing anything in the API, instead just hooking into the thread exit to clear the buffer cache. One thing to watch out for is that the buffer cache may not exist (as the thread didn't do any I/O with heap buffers) so you'll end up creating (an empty) buffer cache at thread exit. That is benign of course but still unsettling to have additional code executing at this time. -Alan
Re: Promptly freeing the per-thread cached direct buffers when a thread exits
Hi David, Thanks for your thoughts. Please see inline. — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com On April 5, 2018 at 6:24:11 PM, David Lloyd (david.ll...@redhat.com) wrote: On Thu, Apr 5, 2018 at 4:45 PM, Tony Printezis wrote: > Would proposing to introduce thread exit hooks be reasonable? Or is > everyone going to freak out? :-) The hooks can be either per-Thread or even > per-ThreadLocal. And it's OK if the hooks can only be used within java.base. User-accessible thread exit hooks would be nice, from a user perspective. From a JDK perspective - it adds new opportunities for user code to jam things up, so it's a tradeoff. …just like finalizers. :-) Yeah, I get that. Which is why I was a bit reluctant to bring it up. Which is why I proposed to only make it available within java.base? ThreadLocal clearing Could you clarify what you mean by ThreadLocal clearing? on thread exit would have been nice back in the beginning, but now I think it would be a fairly substantial behavior change. Adding a new exit() method on ThreadLocal would be better (but not perfect) compatibility-wise, and see prior note about users jamming things up... I like the suggestion to add an overridable exit() method to ThreadLocal. If you want to avoid calling user code by Thread::exit, would adding ThreadLocals (which are tagged appropriately) to a queue for later processing a better approach (similar to the mechanism used for References / ReferencesQueues)? The user can of course create a memory leak by not polling the queue frequently enough. But, that’s also the case for References. And at least user code cannot block Thread::exit. I think that at a minimum, explicitly releasing thread-local NIO direct buffers on thread exit (without introducing a user facing API) is probably safe. I’m also pretty sure it’s safe. I should have mentioned that the thread-local direct buffers are actually explicitly freed when they are not put back in the cache. So, it should be safe to also explicitly free any that are in the cache when the thread exits. Some kind of user-accessible hook would be nice, but I can't imagine it would take anything less than a massive discussion to get there. I’d like to avoid a massive discussion, which is why I proposed to only make it available within java.base. Tony -- - DML
Re: Promptly freeing the per-thread cached direct buffers when a thread exits
Hi David,
ThreadLocals getting cleared is not enough. The threadLocals field is
cleared by Thread::exit:
private void exit() {
...
threadLocals = null;
...
}
but this doesn’t really do anything. The GC has to run, find the direct
buffer in the ThreadLocal unreachable, and queue its Cleaner for
processing. I’m looking for a hook to explicitly reclaim the direct buffer
when Thread::exit is called.
Tony
—
Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
On April 5, 2018 at 6:07:26 PM, David Holmes (david.hol...@oracle.com)
wrote:
Hi Tony,
On 6/04/2018 7:45 AM, Tony Printezis wrote:
> Hi all,
>
> We recently hit another interesting issue with the NIO thread-local
> DirectByteBuffer cache. One of our services seems to create and drop
If it's in a ThreadLocal then aren't you just asking for thread-locals
to be cleared at thread exit?
Cheers,
David
> threads at regular intervals. I assume this is due to a thread pool
> dynamically resizing itself.
>
> Let's say a thread starts, lives long enough for its Thread object to be
> promoted to the old gen (along with its cached direct buffer), then
exits.
> This will result in its cached direct buffer(s) being unreachable in the
> old gen and will only be reclaimed after the next full GC / concurrent GC
> cycle.
>
> Interestingly, the service in question does concurrent GC cycles really
> infrequently (one every few days) as it has a really low promotion rate.
> This results in the JVM's total direct size constantly increasing (which
is
> effectively a native buffer leak).
>
> Has anyone come across this issue before?
>
> There are a few obvious ways to mitigate this, e.g., cause a Full GC /
> concurrent GC cycle at regular intervals. However, the best solution IMHO
> is to explicitly free any direct buffers that are still in the cache when
a
> thread exits.
>
> I'll be happy to implement this and test it internally at Twitter, if
it's
> not on anyone else's radar. However, to do what I'm proposing I need some
> sort of thread exit hook. Unfortunately, there doesn't seem to be one.
>
> Would proposing to introduce thread exit hooks be reasonable? Or is
> everyone going to freak out? :-) The hooks can be either per-Thread or
even
> per-ThreadLocal. And it's OK if the hooks can only be used within
java.base.
>
> FWIW, I did a simple prototype of this (I call the hooks from
Thread::exit)
> and it seems to work as expected.
>
> Any thoughts / feedback on this will be very appreciated.
>
> Thanks,
>
> Tony
>
>
>
> —
> Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
>
Promptly freeing the per-thread cached direct buffers when a thread exits
Hi all, We recently hit another interesting issue with the NIO thread-local DirectByteBuffer cache. One of our services seems to create and drop threads at regular intervals. I assume this is due to a thread pool dynamically resizing itself. Let's say a thread starts, lives long enough for its Thread object to be promoted to the old gen (along with its cached direct buffer), then exits. This will result in its cached direct buffer(s) being unreachable in the old gen and will only be reclaimed after the next full GC / concurrent GC cycle. Interestingly, the service in question does concurrent GC cycles really infrequently (one every few days) as it has a really low promotion rate. This results in the JVM's total direct size constantly increasing (which is effectively a native buffer leak). Has anyone come across this issue before? There are a few obvious ways to mitigate this, e.g., cause a Full GC / concurrent GC cycle at regular intervals. However, the best solution IMHO is to explicitly free any direct buffers that are still in the cache when a thread exits. I'll be happy to implement this and test it internally at Twitter, if it's not on anyone else's radar. However, to do what I'm proposing I need some sort of thread exit hook. Unfortunately, there doesn't seem to be one. Would proposing to introduce thread exit hooks be reasonable? Or is everyone going to freak out? :-) The hooks can be either per-Thread or even per-ThreadLocal. And it's OK if the hooks can only be used within java.base. FWIW, I did a simple prototype of this (I call the hooks from Thread::exit) and it seems to work as expected. Any thoughts / feedback on this will be very appreciated. Thanks, Tony — Tony Printezis | @TonyPrintezis | tprinte...@twitter.com
RFR: 8151098: Introduce multi-slot per-thread cache for StringDecoders/Encoders
Hi all, We discussed this change in a previous e-mail thread. Here’s a patch for your consideration: http://cr.openjdk.java.net/~tonyp/8151098/webrev.1/ I cloned the Cache class from ThreadLocalCoders and reworked it a bit. The StringDecoder and StringEncoder classes had some common fields (the Charset and the requested charset name). I moved them to a superclass (StringCoder) which made the cache easier to write (I didn’t have to create one subclass for the decoder and one for the encoder, as it is the case in ThreadLocalCoders). Feedback very welcome! Tony - Tony Printezis | JVM/GC Engineer / VM Team | Twitter @TonyPrintezis tprinte...@twitter.com
Re: Frequent allocations / promotions of StringCoding$String{Encoder,Decoder} objects
All, Thanks for the replies! Aleksey, Thanks for pointing me to the CR (JDK-4806753). I’m a bit confused though. The CR was resolved as fixed but Mark’s patch doesn’t seem to have made it to the StringCoding.java file (I checked the JDK 8 and 9 source repos; also the class sources in src.zip in 1.5.0_1, the Fixed In release, and for good measure last 5 and 6 releases). However, I noticed that the cache in java.nio.cs.ThreadLocalCoders looks very similar to Mark's cache. Was it decided to maybe cache the CharsetEncoders/Decoders instead of the StringCoding$StringEncoders/Decoders (i.e., was that the fix for JDK-4806753)? Unfortunately, this is pre-mercurial so I can’t get the history on this. Any volunteers at Oracle? One more observation: the change that introduced the SoftReferences: - /* The cached coders for each thread - */ - private static ThreadLocal decoder = new ThreadLocal(); - private static ThreadLocal encoder = new ThreadLocal(); + /** The cached coders for each thread */ + private final static ThreadLocal> decoder = + new ThreadLocal>(); + private final static ThreadLocal> encoder = + new ThreadLocal>(); was done by this changeset: changeset: 18:b5da6145b050 user: martin date: Sun Mar 09 21:56:42 2008 -0700 files: src/share/classes/java/lang/StringCoding.java description: 6671834: (str) Eliminate StringCoding.java compile warnings Reviewed-by: iris Relevant CR: https://bugs.openjdk.java.net/browse/JDK-6671834 There is no explanation of why SoftReferences were introduced. The only comment from Martin is “A fine things to do.” Eliminating compilation warnings is indeed a fine thing to do. Piggy-backing functional changes on that without any explanation is not (FWIW). I personally propose to remove the use of SoftReferences moving forward. Going back to introducing the caches to StringCoding: I can replicate what’s in java.nio.cs.ThreadLocalCoders or I can re-use the ThreadLocalCoders.Cache class. I’d rather do the latter. Any thoughts on what the best way to do this is (move it to a file by itself, make the class public, etc.)? Tony On February 25, 2016 at 4:16:32 PM, Aleksey Shipilev (aleksey.shipi...@oracle.com) wrote: On 02/25/2016 11:48 PM, Tony Printezis wrote: > Has anyone identified this issue before? Hm, there is a blast from the past: https://bugs.openjdk.java.net/browse/JDK-4806753 In comments there, Mark suggests a patch that apparently handles multiple coders. I wonder where did that go. Also, Martin Buchholz is still an assignee there :) > We believe that caching a small number of coders per thread (instead > of just one) could avoid this unnecessary churn. We’ll be happy to > contribute such a fix if there’s interest in getting it accepted. Yes, this seems important. Thanks, -Aleksey - Tony Printezis | JVM/GC Engineer / VM Team | Twitter @TonyPrintezis tprinte...@twitter.com
Frequent allocations / promotions of StringCoding$String{Encoder,Decoder} objects
Hi all,
We recently noticed that in several of our production services there are very
frequent allocations / promotions of coder objects from the StringCoding class
(e.g. java.lang.StringCoding$String{Encoder,Decoder} instances). We are pretty
sure that these are allocated to replace the coders cached in the StringCoding
ThreadLocals (we also see SoftReferences being allocated / promoted along with
the coder objects):
/** The cached coders for each thread */
private final static ThreadLocal> decoder =
new ThreadLocal<>();
private final static ThreadLocal> encoder =
new ThreadLocal<>();
We seem to be doing encoding / decoding using at least two different charsets
(we also see Encoder / Decoders for those two charsets being allocated /
promoted) which is causing this churn (i.e., the coder allocations are not
caused by the SoftReferences being cleared; they are frequently replaced when a
coder for a different charset is required). Even though we observed this
behavior with JDK 8 it should also exist in JDK 9 (StringCoding still has the
same two ThreadLocals; we can confirm this very easily).
Has anyone identified this issue before? We believe that caching a small number
of coders per thread (instead of just one) could avoid this unnecessary churn.
We’ll be happy to contribute such a fix if there’s interest in getting it
accepted.
Also, why are SoftReferences used here? In our case, if a thread does some
encoding / decoding once it’s likely it will likely keep doing it for ever. So
the use of SoftReferences here is not very helpful (it adds unnecessary
overhead to the GC, not only due to the extra copying but also due to the extra
work required during reference processing). Was there a specific reason for the
use of SoftReferences here?
Finally, the StringCoding coder c'tor allocates a new Charset coder
(Charset{Encoder,Decoder}) for the specific charset. But such Charset coders
already seem to be cached in ThreadLocals in the sun.nio.cs.ThreadLocalCoders
class. Any reason why we cannot re-use those? (Oh, and also note that this
cache does not use SoftReferences, which makes their use by the StringCoding
class even more perplexing.)
(a tip of the hat to my colleague Peter Beaman for discovering this issue)
Tony
-
Tony Printezis | JVM/GC Engineer / VM Team | Twitter
@TonyPrintezis
tprinte...@twitter.com
Re: JDK 9 RFR of JDK-8148627: Problem list TestMaxCachedBufferSize.java
Thanks Joe! On January 29, 2016 at 3:13:37 PM, joe darcy (joe.da...@oracle.com) wrote: Hi Alan, Limiting the test to 64-bit platforms is fine with me; I'll push the change as you've suggested later today. Thanks, -Joe On 1/29/2016 11:59 AM, Alan Bateman wrote: > > > On 29/01/2016 18:53, joe darcy wrote: >> Hello, >> >> Until JDK-8148540 is addressed, the test >> >> sun/nio/ch/TestMaxCachedBufferSize.java >> >> should be problem listed on the platforms is fails on. Please review >> the patch below. >> > I think this test will be tricky to be reliable on 32-bit so the > simplest is to just restrict it to 64-bit with: > > @requires (sun.arch.data.model == "64") > > -Alan. > - Tony Printezis | JVM/GC Engineer / VM Team | Twitter @TonyPrintezis tprinte...@twitter.com
