Add contiguous array dataAddr field shadow symbol reference for off heap technology #7150
Conversation
VermaSh
requested review from
vijaysun-omr,
Leonardo2718,
0xdaryl and
mstoodle
as code owners
VermaSh
force-pushed
the
offHeapMemoryCommitAndDecommitFinal_base_changes
branch
from
1fe1e71
to
b618479
Compare
VermaSh
changed the title
|
@0xdaryl @zl-wang @vijaysun-omr Can I please get a review for this PR |
| { | ||
| if (!element(contiguousArrayDataAddrFieldSymbol)) | ||
| { | ||
| TR::Symbol * sym = TR::Symbol::createShadow(trHeapMemory(), TR::Int32); |
There was a problem hiding this comment.
Is there a reason why we use Int32 here ? Isn't the data address pointer 64-bits (on 64-bit platforms which may be the only place where off-heap is done) ?
There was a problem hiding this comment.
Sorry, that should be TR::Address. I have updated it in my latest commit.
VermaSh
force-pushed
the
offHeapMemoryCommitAndDecommitFinal_base_changes
branch
from
b618479
to
6d97876
Compare
|
@vijaysun-omr can I get another set of review? |
compiler/il/OMRNode.cpp
Outdated
| TR_ASSERT_FATAL_WITH_NODE(self(), self()->getOpCode().hasSymbolReference(), "parameter error"); | ||
| retNode = TR::Node::createLoad(self(), self()->getSymbolReference()); | ||
| } | ||
| else if (self()->getReferenceCount() > 0) |
There was a problem hiding this comment.
Is there some assumption that self will be a particular opcode in the else if ? I ask, because duplicateTree can be tricky api to call for complex trees with commoned IL nodes. But if this can only be a direct load for example, then maybe it is fine.
There was a problem hiding this comment.
This actually doesn't need to be checked in. We found that it was causing test failures. I have removed it in my latest commit.
VermaSh
changed the title
VermaSh
changed the title
VermaSh
force-pushed
the
offHeapMemoryCommitAndDecommitFinal_base_changes
branch
from
6d97876
to
5fb24f3
Compare
Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
VermaSh
force-pushed
the
offHeapMemoryCommitAndDecommitFinal_base_changes
branch
from
5fb24f3
to
afc06e0
Compare
|
Launched an other personal build to verify changes |
|
My personal build passed without any failures. Failure in @vijaysun-omr this is ready for review/merge. |
| #if defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION) | ||
| TR::SymbolReference * findOrCreateContiguousArrayDataAddrFieldShadowSymRef(); | ||
| #endif // defined(J9VM_GC_ENABLE_SPARSE_HEAP_ALLOCATION) | ||
| TR::SymbolReference * findContiguousArrayDataAddrFieldShadowSymRef(); |
There was a problem hiding this comment.
Why is the findOrCreate inside an ifdef but the find routine is not ?
There was a problem hiding this comment.
Sorry about the delay, I must have missed the email for comment. But findOrCreate routine uses TR::Compiler->om.offsetOfContiguousDataAddrField API which needs to be guarded by ifdef. However, find isn't guarded because it is used by node->isDataAddrPointer(). I left find outside to avoid having ifdef in node->isDataAddrPointer().
|
jenkins build all |
|
jenkins build win |
Add ContiguousArrayDataAddrFieldShadowSymRef along with needed getter and setter. This is minimum set of off heap changes needed for eclipse-openj9/openj9#18303.