Skip to content
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.

Sign up for GitHub

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

Downgrade isIndexableDataAddrPresent to U_32 #20819

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Downgrade isIndexableDataAddrPresent to U_32 #20819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion debugtools/DDR_VM/src/com/ibm/j9ddr/AuxFieldInfo29.dat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ J9JavaVM.hiddenLockwordFieldShape = required
J9JavaVM.identityHashData = required
J9JavaVM.impdep1PC = U8*
J9JavaVM.initialMethods = required
J9JavaVM.isIndexableDataAddrPresent = UDATA
J9JavaVM.isIndexableDataAddrPresent = U_32
J9JavaVM.intReflectClass = required
J9JavaVM.j2seVersion = required
J9JavaVM.j9ras = required
Expand Down
10 changes: 5 additions & 5 deletions runtime/oti/j9nonbuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5516,8 +5516,8 @@ typedef struct J9VMThread {
UDATA contiguousIndexableHeaderSize;
UDATA discontiguousIndexableHeaderSize;
#if defined(J9VM_ENV_DATA64)
UDATA isIndexableDataAddrPresent;
BOOLEAN isVirtualLargeObjectHeapEnabled;
U_32 isIndexableDataAddrPresent;
U_32 isVirtualLargeObjectHeapEnabled;
#endif /* defined(J9VM_ENV_DATA64) */
void* gpInfo;
void* jitVMwithThreadInfo;
Expand Down Expand Up @@ -6078,9 +6078,9 @@ typedef struct J9JavaVM {
UDATA contiguousIndexableHeaderSize;
UDATA discontiguousIndexableHeaderSize;
#if defined(J9VM_ENV_DATA64)
UDATA isIndexableDataAddrPresent;
BOOLEAN isVirtualLargeObjectHeapEnabled;
BOOLEAN isIndexableDualHeaderShapeEnabled;
U_32 isIndexableDataAddrPresent;
U_32 isVirtualLargeObjectHeapEnabled;
U_32 isIndexableDualHeaderShapeEnabled;
Comment on lines -6081 to +6083
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We haven't shipped this change yet, until the 0.51 release.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the change seems to be causing an -Xint startup perf regression.
https://github.ibm.com/runtimes/javanext/issues/491

#endif /* defined(J9VM_ENV_DATA64) */
struct J9VMThread* exclusiveVMAccessQueueHead;
struct J9VMThread* exclusiveVMAccessQueueTail;
Expand Down