Downgrade isIndexableDataAddrPresent to U_32 #20819
Conversation
Effectively this was used as a BOOLEAN (which is typedef of U_32/I_32), but being UDATA just taking more space than necessary on 64bit platforms. Thus compiler will be able to better pack 2 consecutive boolean fields (isIndexableDataAddrPresent and isVirtualLargeObjectHeapEnabled) Using U_32 rather than BOOLEAN to be able to consistently update DDR code (which seems not to use BOOLEAN) Signed-off-by: Aleksandar Micic <[email protected]>
|
jenkins compile all jdk21 |
|
Pls pass it by a vmfarm build. |
| UDATA isIndexableDataAddrPresent; | ||
| BOOLEAN isVirtualLargeObjectHeapEnabled; | ||
| BOOLEAN isIndexableDualHeaderShapeEnabled; | ||
| U_32 isIndexableDataAddrPresent; | ||
| U_32 isVirtualLargeObjectHeapEnabled; | ||
| U_32 isIndexableDualHeaderShapeEnabled; |
There was a problem hiding this comment.
Type changes like these cause compatibility problems for DDR accessing older system dumps where the fields had different types. See my earlier comment: #20804 (review).
There was a problem hiding this comment.
OK, I see that javaVM.isIndexableDataAddrPresent() was indeed explicitly used in DDR. I don't see explicit usages of the other 2 fields, though.
Soon, we will be removing all these 3 fields and replacing them with one composite field (a bit-wise union), at which point we will be forced to properly implement isIndexableDataAddrPresent() to extract the value from the composite field or try to return the field itself, if it exists, for older cores.
That should resolve potential compatibility problems between DDR and cores for released JVMs (although some non-released JVMs could still experience incompatibilities).
There was a problem hiding this comment.
This only serves to complicate matters as now there are (at least) two kinds of "older cores". It would have been better to leave the fields as they were and switch to the bit-mask in one step.
There was a problem hiding this comment.
We haven't shipped this change yet, until the 0.51 release.
There was a problem hiding this comment.
Also the change seems to be causing an -Xint startup perf regression.
https://github.ibm.com/runtimes/javanext/issues/491
Effectively this was used as a BOOLEAN (which is typedef of U_32/I_32), but being UDATA just taking more space than necessary on 64bit platforms.
Thus compiler will be able to better pack 2 consecutive boolean fields (isIndexableDataAddrPresent and isVirtualLargeObjectHeapEnabled)
Using U_32 rather than BOOLEAN to be able to consistently update DDR code (which seems not to use BOOLEAN)