-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8296024: Usage of DirectBuffer::address should be guarded #11260
Changes from 7 commits
23a6fd1
9fafafa
9fcf2bb
c081b4a
eca7c38
17a72e9
a0de3bc
7097934
88ed3aa
0526e3d
06c764c
88ae68b
682b6f5
0546b23
30aff11
95c4058
eedf41a
619473e
cd95bc8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -778,6 +778,14 @@ final void checkSession() { | |
// setup access to this package in SharedSecrets | ||
SharedSecrets.setJavaNioAccess( | ||
new JavaNioAccess() { | ||
|
||
// We are not using a lambda here because this would create | ||
// a circular dependency with lambda factories. | ||
static final JavaNioAccess.SessionAcquisition NO_OP_CLOSE = new JavaNioAccess.SessionAcquisition() { | ||
@Override | ||
public void close() {} | ||
}; | ||
|
||
@Override | ||
public BufferPool getDirectBufferPool() { | ||
return Bits.BUFFER_POOL; | ||
|
@@ -823,7 +831,7 @@ public MemorySegment bufferSegment(Buffer buffer) { | |
} | ||
|
||
@Override | ||
public Runnable acquireSession(Buffer buffer, boolean async) { | ||
public Runnable acquireSessionOrNull(Buffer buffer, boolean async) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If allocation/performance is an issue, a relatively straightforward way to adjust the code would be to let go of the TWR entirely. E.g. we have code that does this:
We could transform to this:
This leads to a simpler patch that is probably easier to validate. The remaining IOUtil code will be using a different idiom, and perhaps we can, at some point, go back and make that consistent too. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The AutoCloseable experiment was interesting to try but I think you are right, acquire try { .. } finally release would be simpler and also remove the concerns about the performance critical code paths. |
||
var session = buffer.session(); | ||
if (session == null) { | ||
return null; | ||
|
@@ -835,6 +843,16 @@ public Runnable acquireSession(Buffer buffer, boolean async) { | |
return session::release0; | ||
} | ||
|
||
@Override | ||
public SessionAcquisition acquireSession(Buffer buffer) { | ||
var session = buffer.session(); | ||
if (session == null) { | ||
return NO_OP_CLOSE; | ||
} | ||
session.acquire0(); | ||
return session::release0; | ||
} | ||
|
||
@Override | ||
public void force(FileDescriptor fd, long address, boolean isSync, long offset, long size) { | ||
MappedMemoryUtils.force(fd, address, isSync, offset, size); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,8 @@ | |
import jdk.internal.util.Preconditions; | ||
import jdk.internal.vm.annotation.IntrinsicCandidate; | ||
|
||
import static java.util.zip.ZipUtils.NIO_ACCESS; | ||
|
||
/** | ||
* A class that can be used to compute the Adler-32 checksum of a data | ||
* stream. An Adler-32 checksum is almost as reliable as a CRC-32 but | ||
|
@@ -87,6 +89,7 @@ public void update(byte[] b, int off, int len) { | |
* | ||
* @since 1.8 | ||
*/ | ||
@SuppressWarnings("try") | ||
@Override | ||
public void update(ByteBuffer buffer) { | ||
int pos = buffer.position(); | ||
|
@@ -96,7 +99,7 @@ public void update(ByteBuffer buffer) { | |
if (rem <= 0) | ||
return; | ||
if (buffer.isDirect()) { | ||
try { | ||
try (var guard = NIO_ACCESS.acquireSession(buffer)) { | ||
adler = updateByteBuffer(adler, ((DirectBuffer)buffer).address(), pos, rem); | ||
} finally { | ||
Reference.reachabilityFence(buffer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The updated naming is a bit better but there are it still feels that that there are too many things going on at the use sites ("guard", "session", "reachability fence"). Ideally the acquire would return something with an accessor for the direct buffer address but that may not be possible. I wonder if changing NOP_CLOSE.close to do reachabilityFence(this) would work as expected and improve many of these use sites? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can certainly be done. We could, for example, add a new method to the
This would allow us to go from:
to
Although this looks much simpler and concise, it means "a new object is created for each invocation" (*). This also allows us to remove the Here is how the undercarriage might look like:
and
(*) In reality, a representation of the object might be allocated on the stack instead. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
My comment was actually to see if DirectBuffer could extend AutoCloseable so that the acquire returns "this" for the non-session case. Doing the equivalent for the session case might leak into MemorySessionImpl implementing DirectBuffer which you probably don't want to do. If NOP_CLOSE.close can do the Reference.reachabilityFence(this) then it would at least improve some of the use-sites. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure that is simpler. ByteBuffer <: AutoCloseable doesn't seem to make sense to me. I'm also not sure how much object allocation (all this stuff will become value types) should be the driving factor in these code paths. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, I didn't mean BB <: AC but to see if we avoid needing to wrap it because this PR is touching several low level and performance critical code paths. For the faraway places then having the close do a Reference.reachabilityFence(this) should avoid the surprise that the buffer has to kept alive even though it appears that the try-with-resources is doing it already. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have reworked Acquisition. I think we could merge the old and new way in a separate PR. |
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking at the implementation some more, I'm not sure this need fixing? E.g. this method is just using the address to compute some overlap - and return a buffer sliced accordingly. There's no access to the buffer data (except for the last part which does a
put
). The access will fail if the session is closed from underneath. I don't think this can crash the VM (in fact this code did not have a reachability fence to begin with).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted. I will remove the guarding here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
@SuppressWarnings
annotation is no longer needed: