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 helper functions for off heap technology changes #18303

Conversation

VermaSh
Copy link
Contributor

@VermaSh VermaSh commented Oct 18, 2023

Add helper functions to generate IL for array access and queries to check if off heap allocation is enabled. This includes minimum set of off heap changes needed to unblock #17969.

depends on eclipse-omr/omr#7150

@VermaSh VermaSh marked this pull request as draft October 18, 2023 19:58
@VermaSh VermaSh force-pushed the offHeapMemoryCommitAndDecommitFinal_base_changes branch 2 times, most recently from 36afdda to e22fa55 Compare November 3, 2023 04:05
@VermaSh VermaSh changed the title WIP: Off heap technology changes Off heap technology changes Nov 3, 2023
@VermaSh VermaSh marked this pull request as ready for review November 3, 2023 14:33
@VermaSh VermaSh force-pushed the offHeapMemoryCommitAndDecommitFinal_base_changes branch from e22fa55 to b2d8647 Compare November 3, 2023 14:35
@VermaSh VermaSh changed the title Off heap technology changes Helper functinos for off heap technology changes Nov 3, 2023
@VermaSh
Copy link
Contributor Author

VermaSh commented Nov 3, 2023

@dmitripivkine @amicic @0xdaryl @zl-wang @vijaysun-omr Can I please get a review for this PR

@VermaSh VermaSh force-pushed the offHeapMemoryCommitAndDecommitFinal_base_changes branch from b2d8647 to a36d342 Compare November 3, 2023 14:40
@VermaSh VermaSh changed the title Helper functinos for off heap technology changes Helper functions for off heap technology changes Nov 3, 2023
@VermaSh VermaSh changed the title Helper functions for off heap technology changes Add helper functions for off heap technology changes Nov 3, 2023
{
TR_ASSERT_FATAL_WITH_NODE(arrayObject,
TR::Compiler->om.isOffHeapAllocationEnabled(),
"This helpler shouldn't be called if off heap allocation is disabled.\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo "helpler"

* The compilation object
*
* \param arrayNode
* The aray object node
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo "aray"

*
* \param elementSize
* Array element size
*
Copy link
Contributor

@vijaysun-omr vijaysun-omr Nov 4, 2023

Choose a reason for hiding this comment

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

Can we clarify what the strideNode means here ? It seems clearer how to use elementSize with indexNode to come up with an "offset tree" but what role a strideNode plays is less clear because it is not as clearly defined. Maybe an example/pseudocode would help.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updated the comment and variable name. Does it help or should I add more?

Copy link
Contributor

@dmitripivkine dmitripivkine left a comment

Choose a reason for hiding this comment

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

GC part of the change looks good, this is exact replica of off-heap prototype

@VermaSh VermaSh force-pushed the offHeapMemoryCommitAndDecommitFinal_base_changes branch from a36d342 to 2bba7fc Compare November 7, 2023 18:31
@VermaSh
Copy link
Contributor Author

VermaSh commented Nov 7, 2023

Here's my build to verify that this PR doesn't cause any failures

if (TR::Compiler->om.isOffHeapAllocationEnabled())
{
arrayAddressNode = generateDataAddrLoadTrees(comp, arrayNode);
if (offsetNode)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this node also need to be tagged as an internal pointer ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offset node doesn't contain an address; just an offset value relative to the array object pointer. So we should be ok not marking it as an internal pointer.

@VermaSh
Copy link
Contributor Author

VermaSh commented Dec 1, 2023

@vijaysun-omr can I get another set of review?

@vijaysun-omr
Copy link
Contributor

Jenkins test sanity all jdk11

@VermaSh VermaSh changed the title Add helper functions for off heap technology changes WIP: Add helper functions for off heap technology changes Dec 4, 2023
@vijaysun-omr
Copy link
Contributor

@VermaSh please let me know when this is ready to be tested. Thanks

@VermaSh VermaSh force-pushed the offHeapMemoryCommitAndDecommitFinal_base_changes branch from 2bba7fc to d9a0a6b Compare January 19, 2024 15:46
@VermaSh VermaSh requested a review from dsouzai as a code owner January 19, 2024 15:46
@dsouzai
Copy link
Contributor

dsouzai commented Jan 19, 2024

FYI you'll need to update the JITServer MINOR_NUMBER in CommunicationStream.hpp since there's a change in the vmInfo.

@VermaSh VermaSh force-pushed the offHeapMemoryCommitAndDecommitFinal_base_changes branch from d9a0a6b to 4fd4624 Compare January 19, 2024 15:53
@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 19, 2024

Launched an other personal build to verify changes

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 20, 2024

My personal build passed without any failures. @vijaysun-omr this is ready for review/merge.

OMR PR: eclipse-omr/omr#7150 must be merged before this. I'll remove the WIP tag once OMR PR is merged.

@vijaysun-omr
Copy link
Contributor

The OMR PR is merged, but you may need it to get promoted first, before we can merge it. Please remove the WIP and comment when it is ready for review.

@VermaSh VermaSh changed the title WIP: Add helper functions for off heap technology changes Add helper functions for off heap technology changes Jan 24, 2024
@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 24, 2024

@vijaysun-omr This is ready for review. I'll post a comment here once my OMR changes are promoted.

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 25, 2024

@vijaysun-omr OMR changes were promoted. This should be safe to merge.

@vijaysun-omr
Copy link
Contributor

Jenkins test sanity all jdk11

@vijaysun-omr
Copy link
Contributor

Jenkins test sanity zlinux jdk11

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 25, 2024

Failure don't seem to be related to this PR. Probably just need to rebase.

midronij and others added 2 commits January 25, 2024 16:03
Add definitions for j9gc_off_heap_allocation_enabled and
J9::ObjectModel::isOffHeapAllocationEnabled()

Source: eclipse-openj9#18163

Signed-off-by: midronij <[email protected]>
New APIs are:
`generateDataAddrLoadTrees`
`generateArrayAddressNode`
`generateArrayStartTrees`
`generateOffsetNode`

Signed-off-by: Shubham Verma <[email protected]>
@VermaSh VermaSh force-pushed the offHeapMemoryCommitAndDecommitFinal_base_changes branch from 4fd4624 to e766755 Compare January 25, 2024 21:03
@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 25, 2024

@vijaysun-omr Previous build failures should be gone now.

@vijaysun-omr
Copy link
Contributor

Jenkins test sanity all jdk11

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 26, 2024

Build failures aren't related to this PR. They will be fixed in adoptium/TKG#492.

@vijaysun-omr vijaysun-omr merged commit 8a7c85e into eclipse-openj9:master Jan 26, 2024
10 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants