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

Jump to bottom

Cmake build update for off-heap #20461

Merged
merged 1 commit into from
Nov 1, 2024
Merged

Cmake build update for off-heap #20461

merged 1 commit into from
Nov 1, 2024
+26 −14

Conversation

LinHu2016
Copy link
Contributor

@LinHu2016 LinHu2016 commented Oct 31, 2024 •
edited
Loading

new define J9VM_GC_SPARSE_HEAP_ALLOCATION for cmake build

@LinHu2016
Copy link
Contributor Author

@amicic @dmitripivkine please review the changes.Thanks

@amicic
Copy link
Contributor

amicic commented Oct 31, 2024

with this, we will start compiling a lot more code, especially in JIT land
still, in theory everything should be ok, if runtime check is done (which is still set to disabled), too

@amicic
Copy link
Contributor

amicic commented Oct 31, 2024

with this, we will start compiling a lot more code, especially in JIT land still, in theory everything should be ok, if runtime check is done (which is still set to disabled), too

FYI @rmnattas

@amicic
Copy link
Contributor

amicic commented Oct 31, 2024

should we not be updating CR variant of the files too?

@amicic
Copy link
Contributor

amicic commented Oct 31, 2024 •
edited
Loading

I think the flag should be J9VM_GC_SPARSE_HEAP_ALLOCATION (without ENABLE), but it would probably be wise to update all references of it in the code, first

@dmitripivkine do we care, at all?

@dmitripivkine
Copy link
Contributor

dmitripivkine commented Oct 31, 2024 •
edited
Loading

I think the flag should be J9VM_GC_SPARSE_HEAP_ALLOCATION (without ENABLE), but it would probably be wise to update all references of it in the code, first

Agreed with both points. Also agree Compressed specs should be updated as well.
I am neutral about adding this flags now or later.

@LinHu2016
Copy link
Contributor Author

LinHu2016 commented Oct 31, 2024 •
edited
Loading

should we not be updating CR variant of the files too?

looks CR variants would include nonCR ones,

such as

include("${CMAKE_CURRENT_LIST_DIR}/cmprssptrs.cmake")
include("${CMAKE_CURRENT_LIST_DIR}/linux_x86-64.cmake")

@amicic
Copy link
Contributor

amicic commented Oct 31, 2024 •
edited
Loading

OK, then we just need to rename it (as part of this PR), both cmake references and a few that we have in GC code.

For JIT renames, I'm now thinking it's actually better to do it after these changes, since it will defer the point when JIT code under that flag will start compiling, so they can deal with potential problems at that time (and fix as a part of their rename PR).

@amicic
Copy link
Contributor

amicic commented Oct 31, 2024 •
edited
Loading

btw, I'll mention this: eclipse-omr/omr#7513
but problems that are seen there in CI compiles should not stop us from delivering this, as long as we rename the flag (hence nobody is affected by the new name)

amicic
amicic reviewed Oct 31, 2024
View reviewed changes
runtime/cmake/caches/aix_ppc-64.cmake
@@ -21,18 +21,19 @@
################################################################################

#TODO: Env vars should be auto detected by platform
# Prevent CMake from automatically creating export lists for shared libraries
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps leave extra space after the TODO line, which I don't know what it really refers, but like this it looks like it refers to the next 'set' line only
perhaps we can even remove the TODO line!?

@LinHu2016 LinHu2016 mentioned this pull request Oct 31, 2024
Merged
amicic
amicic reviewed Oct 31, 2024
View reviewed changes
runtime/cmake/caches/win_x86.cmake Outdated
@@ -24,6 +24,7 @@ set(J9VM_ARCH_X86 ON CACHE BOOL "")
set(J9VM_ENV_DATA64 OFF CACHE BOOL "")
set(J9VM_ENV_LITTLE_ENDIAN ON CACHE BOOL "")
set(J9VM_ENV_SSE2_SUPPORT_DETECTION ON CACHE BOOL "")
set(J9VM_GC_SPARSE_HEAP_ALLOCATION OFF CACHE BOOL "")
Copy link
Contributor

Choose a reason for hiding this comment

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

not a 64bit spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so we set J9VM_GC_SPARSE_HEAP_ALLOCATION OFF instead of ON?

Copy link
Contributor

Choose a reason for hiding this comment

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

simplest is just remove the line - we did not update any other 32bit for offheap

@midronij midronij mentioned this pull request Oct 31, 2024
Open
@LinHu2016
Cmake update for off-heap
d78ca01 
new define J9VM_GC_SPARSE_HEAP_ALLOCATION for cmake build

Signed-off-by: lhu <linhu@ca.ibm.com>
@amicic
Copy link
Contributor

amicic commented Nov 1, 2024

jenkins compile all jdk21

@amicic
Copy link
Contributor

amicic commented Nov 1, 2024

jenkins compile all jdk8

@amicic
Copy link
Contributor

amicic commented Nov 1, 2024

jenkins test sanity xLinux jdk21

@amicic amicic merged commit 17b3391 into eclipse-openj9:master Nov 1, 2024
21 checks passed
keithc-ca
keithc-ca reviewed Nov 4, 2024
View reviewed changes
runtime/cmake/caches/aix_ppc-64.cmake
set(CMAKE_XL_CreateExportList "" CACHE INTERNAL "")

set(J9VM_GC_TLH_PREFETCH_FTA OFF CACHE BOOL "")
set(J9VM_GC_SPARSE_HEAP_ALLOCATION ON CACHE BOOL "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to change all the references in JIT code from J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION to J9VM_GC_SPARSE_HEAP_ALLOCATION?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. @rmnattas, apologies for the inconvenience, but please update the JIT references.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it also in the plan to update UMA builds to support this flag?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, about to merge that PR.

@rmnattas rmnattas mentioned this pull request Nov 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keithc-ca keithc-ca keithc-ca left review comments

@amicic amicic amicic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LinHu2016 @amicic @dmitripivkine @keithc-ca