-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
8331865: Consolidate size and alignment checks in LayoutPath #19251
Conversation
👋 Welcome back mcimadamore! A progress list of the required criteria for merging this PR into |
@mcimadamore This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 162 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
@mcimadamore |
TODO: use extra MH coordinate to inject check inside VH code
Add comment
/csr |
@mcimadamore has indicated that a compatibility and specification (CSR) request is needed for this pull request. @mcimadamore please create a CSR request for issue JDK-8331865 with the correct fix version. This pull request cannot be integrated until the CSR request is approved. |
@@ -823,6 +828,7 @@ public MemorySegment get(AddressLayout layout, long offset) { | |||
@ForceInline | |||
@Override | |||
public void set(AddressLayout layout, long offset, MemorySegment value) { | |||
Objects.requireNonNull(value); |
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 has been added, otherwise it would be possible to pass a null
as the value of value
, and not get an NPE, in case e.g. the alignment of the segment is incorrect (because we now check that before we even try to perform the memory access).
assertTrue(compatible || | ||
(layout instanceof GroupLayout && segment.maxByteAlignment() < layout.byteAlignment())); | ||
// access is unaligned | ||
assertTrue(segment.maxByteAlignment() < layout.byteAlignment()); |
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.
Note: this change is required because, before this PR, we used to issue UOE for a bad access mode regardless of the alignment with which we accessed the segment (well, only for toplevel var handles). Now we uniformly check alignment before access mode, for both toplevel and nested var handles, so this assertion needed to be tweaked.
@@ -97,9 +97,9 @@ final class VarHandleSegmentAs$Type$s extends VarHandleSegmentViewBase { | |||
#end[floatingPoint] | |||
|
|||
@ForceInline | |||
static AbstractMemorySegmentImpl checkAddress(Object obb, long offset, long length, boolean ro) { | |||
static AbstractMemorySegmentImpl checkReadOnly(Object obb, boolean ro) { |
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 and the following methods are the bulk of the changes in this template. That is, we no longer check size and alignment of the accessed segment. Every other change in this template is needed to get rid of fields and parameters that are no longer used.
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.
Note: we don't even check for overflows, e.g. offset < 0
. The reasoning here is that all layouts check for overflow on construction, so it's never possible to construct a layout whose size is greater than Long.MAX_VALUE
. This constraint also affects computation for indexed var handles, since all the long
indices corresponding to open path elements are checked against their bounds (and their bounds must be small enough so that the enclosing layout has a size smaller than Long.MAX_VALUE
).
Webrevs
|
Do we have any performance tests available to see if there are any potential impacts? |
/** alignment constraint (in bytes, expressed as a bit mask) **/ | ||
final long alignmentMask; | ||
|
||
VarHandleSegmentViewBase(VarForm form, boolean be, long length, long alignmentMask, boolean exact) { | ||
VarHandleSegmentViewBase(VarForm form, boolean be, long alignmentMask, boolean exact) { |
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.
Nit: Copyright year. This also applies to some other files in this PR.
I've run all micro benchmarks whose name match |
Results here: https://cr.openjdk.org/~mcimadamore/jdk/8331865/ |
@@ -45,7 +45,7 @@ public class TestHeapAlignment { | |||
public void testHeapAlignment(MemorySegment segment, int align, Object val, Object arr, ValueLayout layout, Function<Object, MemorySegment> segmentFactory) { | |||
assertAligned(align, layout, () -> layout.varHandle().get(segment, 0L)); | |||
assertAligned(align, layout, () -> layout.varHandle().set(segment, 0L, val)); | |||
MemoryLayout seq = MemoryLayout.sequenceLayout(10, layout); | |||
MemoryLayout seq = MemoryLayout.sequenceLayout(1, layout); |
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 change was an actual issue in the test, which was made manifest by the new checks
* accessed memory segment.</li> | ||
* <li>The access operation must fall inside the spatial bounds of the accessed | ||
* memory segment, or an {@link IndexOutOfBoundsException} is thrown. This is the case | ||
* when {@code B + A <= S}, where {@code O} is the base offset (defined above), |
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.
Do you mean {@code O + A <= S}
?
(Still working my way through the changes...)
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.
Yes, apologies
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.
Took me a few passes to work it all out. Looks good. All the bounds checking action now consistently passes through the call to checkEnclosingLayout
in adaption of the raw (and unsafe) segment accessing VarHandle.
Separately, we might be missing a few long argument accepting guard methods for simpler cases as I suspect they are still focused more on int index types.
Not sure I understand what guard methods you are referring to here? |
Ah, got it. You mean more support in VarHandleGuards. Yes, I have a separate patch for that (actually had that for quite a while), but we're not super sure how to evaluate what impact it has :-) |
Ah, i did not realize that. Yes its tricky, it was designed for VHs to fields/arrays, to really minimize their overhead. With segments there is already some additional overhead e.g.,
So perhaps it does not make much difference. |
True, the chain for segment var handle is quite long (and that is not a result of this patch, it has always been more or less like that). Funnily, I was staring at this piece of code the other day, and I think this can be dealt with morally with a |
} | ||
|
||
@ForceInline | ||
static $type$ get(VarHandle ob, Object obb, long base) { | ||
VarHandleSegmentViewBase handle = (VarHandleSegmentViewBase)ob; | ||
AbstractMemorySegmentImpl bb = checkAddress(obb, base, handle.length, true); | ||
AbstractMemorySegmentImpl bb = checkReadOnly(obb, true); |
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.
For getter methods, which pass a constant true
here, checkReadOnly
essentially just does a null check and cast on the segment. Not sure if it's worth simplifying... (I'm happy if you want to leave it like this as well)
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.
I'll leave it for now. There's always a trade-off with these generated templates...
/integrate |
Going to push as commit c003c12.
Your commit was automatically rebased without conflicts. |
@mcimadamore Pushed as commit c003c12. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Here's a link to the results: |
When creating a nested memory access var handle, we ensure that the segment is accessed at the correct alignment for the root layout being accessed. But we do not ensure that the segment has at least the size of the accessed root layout. Example:
If I have a memory segment whose size is 12, I can successfully call X_HANDLE on it with index 1, like so:
This seems incorrect: after all, the provided segment doesn't have enough space to fit two elements of the nested structs.
This PR adds checks to detect this kind of scenario earlier, thus improving usability. To achieve this we could, in principle, just tweak
LayoutPath::checkEnclosingLayout
and add the required size check.But doing so will add yet another redundant check on top of the other already added by pull/19124. Instead, this PR rethinks how memory segment var handles are created, in order to eliminate redundant checks.
The main observation is that size and alignment checks depend on an enclosing layout. This layout might be the very same value layout being accessed (this is the case when e.g. using
JAVA_INT.varHandle()
). But, in the general case, the accessed value layout and the enclosing layout might differ (e.g. think about accessing a struct field).Furthermore, the enclosing layout check depends on the base offset at which the segment is accessed, prior to any index computation that occurs if the accessed layout path has any open elements. It is important to notice that this base offset is only available when looking at the var handle that is returned to the user. For instance, an indexed var handle with coordinates:
Is obtained through adaptation, by taking a more basic var handle of the form:
And then injecting the result of the index multiplication into
offset
. As such, we can't add an enclosing layout check inside the var handle returned byVarHandles::memorySegmentViewHandle
, as doing so will end up seeing the wrong offset (e.g. an offset in which the index part has already been mixed in).The only possibility then, is to remove size and alignment checks from the raw var handles returned by
VarHandles::memorySegmentViewHandle
, and perform such checks outside (e.g. inLayoutPath::dereferenceHandle
). The only checks left in the raw var handles are:Since these check depends on the particular access mode selected by the client, we can't move these checks away from the raw var handle.
These changes come with some consequences:
ValueLayouts
also need to call the path layout variant ofMemoryLayout::varHandle
, to make sure that the raw var handle is adapted accordingly, before it is cached in its stable field.Utils
toValueLayouts::varHandle
. The cache is used (a) to reduce the number of var handle instances created and (b) to make sure that the cached var handle in theValueLayout
has stable identity. With respect to (a), while it makes sense to cache "toplevel" var handles (e.g.JAVA_INT.varHandle()
) the cost/benefit ratio for caching nested var handles seem more unfavourable, as the same var handle can be reused with different enclosing layouts, leading to different checks. Ultimately, all nested var handles will require some kind of adaptation, so it doesn't seem too crucial to have a deeper level of caching here.MemoryLayout.sequenceLayout(Long.MAX_VALUE, JAVA_LONG)
, and derive an indexed var handle from that layout. Since there used to be no enclosing layout size check, access to the sequence element was allowed, as long as the index computation did not result in an offset outside the boundary of the accessed memory segment. This is now no longer the case: when selecting an element from the above layout, the implementation will make sure that the accessed segment indeed has the size of that sequence layout (which will probably lead to aIndexOutOfBoundException
). To do indexed access on an unbounded sequence, theMemoryLayout::arrayElementVarHandle
should be used instead (but this is the norm anyway for such cases).Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19251/head:pull/19251
$ git checkout pull/19251
Update a local copy of the PR:
$ git checkout pull/19251
$ git pull https://git.openjdk.org/jdk.git pull/19251/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19251
View PR using the GUI difftool:
$ git pr show -t 19251
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19251.diff
Webrev
Link to Webrev Comment