On 10/07/2013 02:43 PM, Aleksey Shipilev wrote:
Hi Peter,
On 10/07/2013 02:56 AM, Peter Levart wrote:
http://cr.openjdk.java.net/~plevart/jdk8-tl/DyrectBufferAlloc/webrev.01/
The 1 hour run of this on my 1x2x2 i5, directMem=16m, original
DirectByteBufferTest yields no failures! Oh my. Good job!
Thanks!
Comments on the code:
Bits.java:
- Thread.currentThread().interrupt() only sets the interruption flag,
it does not break the control flow. You can call it directly in the
exception handler, sparing some boiler-plate code.
...then the next possible sleep() in loop will immediately throw
InterruptedException again. That's why I set the flag and set the
interrupted status just before returning in a finally block where all
exit paths converge...
- Given that you about to retry, do you think catching the interrupt
during sleep should immediately return? Or, try the last time to reserve
some memory, and either return or throw OOME?
I have a rule that when InterruptedException is caught in a method (or
method invokes Thread.interrupted() which returns true), then this
method must do one of the following 3 things:
- throw InterruptedException or
- complete normally or throw any other exception, but make sure current
Thread's interrupted status is set before completing/throwing or
- never complete
That's the only way to not swallow interrupts.
Now in this concrete example, I must not loop indefinitely, I can not
throw InterruptedException, neither can I complete normally, since that
would mean the memory has been reserved (which it hasn't). I could throw
OOME *and* set the interrupted status of current thread, but as we have
seen from the DirectBufferTest I ran, some allocations did need
occasional sleep(1) or even sleep(2) before completing normally. If
thread had interrupted status set in such situations, allocation would
throw OOME, which feels wrong, since it would not be thrown if the
thread was not interrupted.
So it seems best to continue processing and pretend that
InterruptedException never happened, but make sure interrupted status is
set before completing.
What I should correct is, that when the InterruptedException is caught
during sleep, 'sleeps' counter is not incremented and 'sleepTime' is not
doubled, since the wake-up was premature and should not count - 9
consecutive interrupts could theoretically provoke OOME because the loop
would exit after 9 "fake" sleeps.
- Please avoid inline assignments.
Doug's school ;-)
You mean the following:
long totalCap;
while (cap <= maxMemory - (totalCap = totalCapacity.get())) {
if (totalCapacity.compareAndSet(totalCap, totalCap + cap)) {
...
return true;
}
}
So how would you write this? Alternatives:
a)
while (true) {
long totalCap = totalCapacity.get();
if (cap > maxMemory - totalCap) {
break;
}
if (totalCapacity.compareAndSet(totalCap, totalCap + cap)) {
...
return true;
}
}
b)
long totalCap = totalCapacity.get();
while (cap <= maxMemory - totalCap) {
if (totalCapacity.compareAndSet(totalCap, totalCap + cap)) {
...
return true;
}
totalCap = totalCapacity.get();
}
c)
for (long totalCap = totalCapacity.get();
cap <= maxMemory - totalCap;
totalCap = totalCapacity.get()) {
if (totalCapacity.compareAndSet(totalCap, totalCap + cap)) {
...
return true;
}
}
d) ???
- I'd like to embed the comment why 9 is the magic number.
Ok. WIll do.
Reference.java:
- Please move the static initializer block back, that will be cleaner
change.
I actually haven't moved the static initializer block. It's still
immediately after the ReferenceHandler class (at least syntactically). I
moved the content of the for loop in ReferenceHandler#run() method out
into a new method inserted after the static initializer. Sdiffs tool
matches by similarity of content when aligning left and right panes. I
can insert the new method between ReferenceHandler class and static
initializer, thus "moving" the static initializer, and Sdiffs will show
less blue text, but the hg changeset might be even more confusing to
read (or merge) that way. I will try and see what happens.
- Please keep the (r instanceof Cleaner) block as the separate if;
makes it visually different from the common code path.
Right.
DirectBufferAllocTest.java:
- "for(;;)" -> "while(true)"
Ok.
- Can we turn the 60*1000 into the constant?
Ok.
- I think 10 seconds is enough for regression test.
I think too. It usually fails after few seconds.
- Can we fold the relevant into Integer.getInteger("...", #const) to
make them tunable, while still claiming it to be the regtest in the
default mode? Put the comment which settings should be used to turn this
test into the stress test.
WIll do.
Total of 909960 allocations were performed:
- 247993 were satisfied before attempting to help ReferenceHandler thread
- 660184 were satisfied while helping ReferenceHandler thread but before
triggering System.gc()
- 1783 were satisfied after triggering System.gc() but before doing any
sleep
- no sleeping has been performed
The same test, just changing -XX:MaxDirectMemorySize=128m (that means
1MB per thread each allocating direct buffers randomly sized between
256KB and 1MB):
Total of 579943 allocations were performed:
- 131547 were satisfied before attempting to help ReferenceHandler thread
- 438345 were satisfied while helping ReferenceHandler thread but before
triggering System.gc()
- 10016 were satisfied after triggering System.gc() but before doing any
sleep
- 34 were satisfied after sleep(1)
- 1 was satisfied after sleep(1) followed by sleep(2)
Thank you! I was meant to do this for my original patch. (In fact I did
some debug printing on the recovery paths, only to see we almost never
hit them).
So what do you think? Is this still too risky for JDK8?
I think that the patch is great. We are near the JDK 8 endgame though.
As much as I will be happy to have that in 8u0, this is probably
something for early JDK 9, with a backport to 8u2?
I see. JDK9 and backport then.
-Aleksey.
Regards, Peter
