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.

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

Add flag isVirtualLargeObjectHeapEnabled in J9VMThread and J9JavaVM #20503

Merged
merged 1 commit into from
Nov 5, 2024

Conversation

LinHu2016
Copy link
Contributor

  • cache flag isVirtualLargeObjectHeapEnabled in both J9VMThread and J9JavaVM.
  • initialize both isVirtualLargeObjectHeapEnabled flags.

 - cache flag isVirtualLargeObjectHeapEnabled in both J9VMThread and
 J9JavaVM.
 - initialize both isVirtualLargeObjectHeapEnabled flags.

Signed-off-by: lhu <[email protected]>
@LinHu2016
Copy link
Contributor Author

@amicic @dmitripivkine please review the changes, Thanks

@@ -5480,6 +5480,7 @@ typedef struct J9VMThread {
UDATA discontiguousIndexableHeaderSize;
#if defined(J9VM_ENV_DATA64)
UDATA isIndexableDataAddrPresent;
BOOLEAN isVirtualLargeObjectHeapEnabled;
Copy link
Contributor

@amicic amicic Nov 5, 2024

Choose a reason for hiding this comment

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

we could have this flag under the matching compile time flag, which would mean that appropriate access macros that we later introduce would be under the same flag.

@dmitripivkine, opinion, do we want it?

Copy link
Contributor

@amicic amicic Nov 5, 2024

Choose a reason for hiding this comment

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

after some discussion, seems like call sites (array access macros) will be a bit more complex and harder to read (and they are already hard to read), with little benefit (one less field in VM Thread, if build flag is disabled, which would be interesting only for 32bit platforms in some exceptional build specs)
so let's leave it as-is, but we can always add it later if we want

Copy link
Contributor

Choose a reason for hiding this comment

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

With the addition of this field, should line 5517 change?

#if 0 && !defined(J9VM_ENV_DATA64) /* Change to 0 or 1 based on number of fields above */

FYI @gacholio

Copy link
Contributor

Choose a reason for hiding this comment

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

We should add an assertion into the build process to detect this,

Copy link
Contributor

Choose a reason for hiding this comment

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

Was the alignment resolved?

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like that line should be updated only for non 64 bit platforms, and we added a field for 64 bit only.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I see that now. I still think @gacholio's suggestion to have a build-time check is a good one.

@amicic
Copy link
Contributor

amicic commented Nov 5, 2024

jenkins compile aix jdk8

@amicic amicic merged commit c03a349 into eclipse-openj9:master Nov 5, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants