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

Cmake build update for off-heap #20461

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

LinHu2016
Copy link
Contributor

@LinHu2016 LinHu2016 commented Oct 31, 2024

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

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

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

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

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

@amicic
Copy link
Contributor

amicic commented Oct 31, 2024

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)

new define J9VM_GC_SPARSE_HEAP_ALLOCATION for cmake build

Signed-off-by: lhu <[email protected]>
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.

3 participants