JDK-8314882 : Relax alignment of array elements
  • Type: CSR
  • Component: hotspot
  • Sub-Component: runtime
  • Priority: P4
  • Status: Closed
  • Resolution: Approved
  • Fix Versions: 23
  • Submitted: 2023-08-23
  • Updated: 2024-02-15
  • Resolved: 2024-02-15
Related Reports
CSR :  
JDK-8139457 - Relax alignment of array elements
Relates :  
JDK-8318966 - Some methods make promises about Java array element alignment that are too strong
Relates :  
JDK-8318967 - (bf) ByteBuffer::alignedSlice and alignmentOffset should not promise heap alignment
Description
Summary
-------

The alignment of array elements with a size smaller than or equals 4 bytes on 64-bit platforms is relaxed such that array elements can start at 4-byte aligned address, rather than 8-byte aligned address. This allows to utilize the alignment gap between the array-length-field and the start of the array elements, when running with -UseCompressedClassPointers and in the future with +UseCompactObjectHeaders.

Problem
-------

Currently, arrays are laid out in memory like this (each line representing 8 bytes/ 1 word, each block representing 4 bytes):

```
+-----------------------------------------+
| Mark-word                               |
+--------------------------+--------------+
| Compressed Class Pointer | Array length |
+--------------------------+--------------+
| Array elements...                       |

```

However, when running without compressed class pointers (-UseCompressedClassPointers), it looks like this:

```
+-----------------------------------------+
| Mark-word                               |
+-----------------------------------------+
| Class Pointer                           |
+----------------------+------------------+
| Array length         | Alignment gap    |
+----------------------+------------------+
| Array elements...                       |

```

Similarily, with the upcoming JEP 450: Compact Object Headers, it would look like this:

```
+-----------------------------------------+
| Mark-word / CompressedClassPointer      |
+----------------------+------------------+
| Array length         | Alignment gap    |
+----------------------+------------------+
| Array elements...                       |

```

It would be better to use the 4-bytes alignment gap for the array elements, instead:

```
+-----------------------------------------+
| Mark-word / CompressedClassPointer      |
+----------------------+------------------+
| Array length         | Array elements   |
+----------------------+------------------+
| Array elements...                       |

```
 
Solution
--------

The proposed change relaxes the alignment requirements for array types with element sizes of 4 bytes or less - byte[], boolean[], char[], short[], int[], float[] and Object[] (when running with -XX:+UseCompressedOops, which is the default for many heap configurations) such that the array elements can start at 4-byte aligned addresses. long[], double[] and Object[] (with -COOPS) remain unaffected.

Specification
-------------

Not sure how to specify this. The PR has all the relevant changes, but is quite large and not very useful, I suppose. The above problem and solution description captures the change better, IMO.

https://patch-diff.githubusercontent.com/raw/openjdk/jdk/pull/11044.diff
Comments
Moving to Approved contingent on a release note being written to describe the behavior change.
15-02-2024
The related CSR JDK-8320247 has been approved for 23, therefore I am finalizing this CSR.
13-02-2024
[~rkennke], strictly from a CSR process perspective, a Provisional CSR is part-way through the CSR process and the assignee (or other parties working on the CSR) are responsible for changing its state. Common state changes would be to Finalize the request for the second phase of review or to re-Propose the CSR if the contents have changed significantly since the initial review. In this case, the related CSR JDK-8320247 was just Proposed a few days ago. HTH
05-02-2024
Ping. What is the status of this CSR and related issues? I would like to integrate the dependent change as soon as possible (preferably in JDK23 release). What needs to be done before that can happen?
05-02-2024
I think we can separate out this issue/CSR (which can focus on the VM flags etc) from a dependent CSR related to the methods. IMO the latter does not need to block the former in terms of integration, but they do need to be integrated into the same release.
16-11-2023
There are existing spec and implementation bugs in the methods mentioned in the linked issue: https://bugs.openjdk.org/browse/JDK-8318966 Differences between different JVM implementations that are admissible by the JVM spec may be observed through those APIs. This CSR proposes to change the JVM implementation in a manner that is compatible with the JVM spec, so should be allowed. We should instead fix the bugs in those methods so they do not leak details about the particular JVM implementation that is being used.
16-11-2023
Yes, [~psandoz] and [~jvernee], please explicitly state your current analysis of this CSR vis a vis the methods in question. Thanks.
16-11-2023
[~darcy] Ok. As far as I can tell, the remaining concerns around the methods mentioned above are not specific to this change. As far as I understand, this change brought up the discussion about them and we all looked closer and found that *maybe* they overpromise alignment that the JVM cannot guarantee (and, to be honest, I still cannot see how this would be the case). But, as far as I understand, those are general concerns, not something that this change affects. All that this change does is change the alignment of 'small' (i.e. <= 4 bytes) array elements (when not using compressed class-pointers) from 24 to 20 (and with Lilliput to 12 rather than 16). This alignment change is perfectly reflected by the above mentioned methods, I have verified this using various tests that already exist in the JDK. I believe that is why [~jvernee] said above that those concerns will be discussed separately and this change can go in as it is. Please confirm with [~jvernee] and [~psandoz] if what I said is correct.
15-11-2023
[~rkennke], ah okay, I had overlooked the JDK-8318967 link. My overall sentiment stands -- I think any concerns/bugs related to the byteArrayViewVarHandle and similar methods should be resolved before this work can go in.
15-11-2023
Sorry but I don't quite follow. All the mentioned methods would still work as specified and as expected after this change. Ultimately, all the methods rely on ByteBuffer::alignmentOffset, and that method (still) does the right thing, which is, return an offset which can be used to determine an aligned index within a Buffer (or array, VarHandle, etc). That is not broken by this change. I believe we concluded that there may be a more general problem in the *specification* of those methods, where we over-promise something about alignment, but, to be honest, I don't even see that. At least nothing which would be affected by the change that we are discussing here (in other words, if there is an over-promise of alignment at certain offsets, then that problem is pre-existing and unrelated to this change). That is why [~ jvernee] opened the linked issues to discuss the problems there.
15-11-2023
Thanks for the analysis [~jvernee]. I'll moving this CSR to Provisional. However, I won't move it to Approved until there is a clear plan for addressing the methods that wouldn't work after this change.
15-11-2023
Sorry for the delay. I think we have concluded that this change does not violate the VM spec, and that the issue is with several methods [1] which try to promise more alignment of Java array elements than the VM actually guarantees (see the linked issues as well). i.e. this change can go ahead, and we will do something else to address the issues in those methods. [1]: namely: MethodHandles::byteArrayViewVarHandle, MethodHandles::byteBufferViewVarHandle, ByteBuffer::alignmentOffset and ByteBuffer::alignedSlice
14-11-2023
Does anything else need to be done here? Can it be approved?
09-11-2023
Just keep the CSR title to match the originating bug title :-)
24-10-2023
Can we change the CSR title to describe what the change is, rather than the issue? Something like "Remove dependence on HeapWord Granularity for Aligning Elements in Array Objects" Something like that. That's a bit long.
24-10-2023
> when running with -UseCompressedClassPointers and in the future with +UseCompressedClassPointers. From description, should this say +UseCompactObjectHeaders ?
20-10-2023
[~psandoz] I checked the X-Buffer.alignmentOffset() and alignedSlice() and I believe those methods already do the right thing, because they ultimately rely on Unsafe.arrayBaseOffset() which also does the right thing. JTreg tests in java/nio/Buffer and java/lang/invoke/VarHandles are also passing, and from the looks of it, they do check those methods as well. Can you let me know what else might be needed for this CSR to move forward?
16-10-2023
I shall mention that with the proposed change, array elements would still be aligned according their size, e.g. byte[] would have elements at 1-byte-alignment, int[] at 4-bytes and long[] at 8-bytes, etc. In-fact, all the smaller-type arrays would still have their elements at 4-bytes alignment. The problem only arises with 8-byte-typed (long[] and double[]) views of byte[], which - when attempting operations on it which require alignment like CAS - may now throw IllegalStateException when it has previously worked (and vice versa: now work when previously have thrown an ISE). For an example that illustrates the problem (which may well not actually be a problem, because user code should not rely on particular alignments like this, IMO) see: https://github.com/openjdk/jdk/pull/11044#issuecomment-1689544136
25-09-2023
> Similar to that, arrays that are fetched via GetPrimitiveArrayCritical, and operated upon under the assumption of particular alignment (e.g. using SIMD instructions) might break. Have you run into a real-world example of this? I'd expect robust SIMD loops to have pre and post alignment loops. Unless perhaps they are relying on the fact that `malloc` tends to return 16-byte alignment blocks.
25-09-2023
For FFM, the alignments we assume for heap arrays is already strict. e.g. for a byte[] we assume the elements are at most 1-byte aligned. For short, 2-byte, etc. For long and double, we assume at most 4-byte alignment on 32-bit platforms. For unaligned accesses, only the plain get/set access modes are supported, and others result in UnsupportedOperationException. So, these changes should not create any trouble for the FFM API. (see also [2], and the layout constants at [3]) For MethodHandles::byteArrayViewVarhandle I think the same rules should apply. I'll try and dig something up, but in the past we've had lengthy discussions with John about which alignment the VM guarantees for array elements, and ended up settling on the current alignments (i.e. assumed alignment may never be greater than MIN(ADDRESS_BYTE_SIZE, ELEMENT_BYTE_SIZE)). Looking at the javadoc of byteArrayViewVarhandle [1], it doesn't outright claim that accesses are always assumed to be unaligned. But, I think it should, similarly to what we did for FFM. A potential intermediate step is to tighten up the spec for this method to say that it only supports plain get/set, and will throw an ISE otherwise? The doc currently says: "Access of bytes at an index may be aligned or misaligned for T, with respect to the underlying memory address, A say, associated with the array and index.", Since the address of the array can change, and the VM/JVMS makes no guarantees about the alignment of the array elements, I believe that we are in our right to always throw the specified ISE? (for access modes other than get/set) [1]: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/invoke/MethodHandles.html#byteArrayViewVarHandle(java.lang.Class,java.nio.ByteOrder) [2]: https://github.com/openjdk/panama-foreign/pull/876 [3]: https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/foreign/ValueLayout.html#JAVA_BYTE
25-09-2023
And also [~jvernee]. We also need to look at methods on `ByteBuffer`, `alignmentOffset` and `alignedSlice`, including in these cases considering deprecation (not for removal). We could defer to `MemorySegment`, at least in the off-heap cases, where we have better control over alignment. For the VH case we can also refer to `MemorySegment`, again at least in the off-heap cases. Thankfully MS enables *scribbling* on `long[]` for other primitive smaller sized element types, thereby we get on-heap alignment stability.
25-09-2023
I think [~pminborg] can help review this.
25-09-2023
Hmm... Administratively, I'm moving this request back to Draft. I'd like to see some reviewers on it before it moves forward. [~mchung] or [~psandoz], can you take a look or suggest someone else review and suggest what spec changes would be needed to support the proposed changes? Thanks.
28-08-2023