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

NULL initialize dataAddr field for 0 size arrays #20892

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

Conversation

VermaSh
Copy link
Contributor

@VermaSh VermaSh commented Jan 8, 2025

Update array inline allocation sequence to initialize dataAddr field only for non-zero size arrays. Field should be left blank for zero size arrays. Additionally this also clears padding field after the size field.

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 8, 2025

@hzongaro Can I please get a review?

@hzongaro hzongaro self-assigned this Jan 8, 2025
@hzongaro hzongaro self-requested a review January 8, 2025 20:38
Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

I have some questions about whether the padding field always exists.

Also, I wonder whether it would be worthwhile having methods that will return the offset of the padding field instead of calculating it here, and perhaps a method that will indicate whether the padding field even exists, if it might not exist.

runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 22, 2025

That's a good suggestions. I'll open a separate PR with APIs to indicate whether padding field exists and get it's offset. I'll use this PR just for dataAddr changes since it must be cleared for off-heap but we can get away by not clearing padding field.

@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 22, 2025

@hzongaro I have moved my padding field changes to #20998. Can I please get another review for the remaining changes?

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

One small bug and a pair of suggestions.

runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
@VermaSh VermaSh force-pushed the inline_array_allocation_x branch 2 times, most recently from 201b79a to 56c5ec4 Compare January 29, 2025 15:04
@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 29, 2025

@hzongaro I have updated the PR with your suggestions. Can I please get another review?

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good. One minor suggestion.

runtime/compiler/x/codegen/J9TreeEvaluator.cpp Outdated Show resolved Hide resolved
Update array inline allocation sequence to initialize dataAddr field
only for non-zero size arrays. Field should be left blank for zero
size arrays.

Signed-off-by: Shubham Verma <[email protected]>
@VermaSh VermaSh force-pushed the inline_array_allocation_x branch from 56c5ec4 to 216efba Compare January 29, 2025 19:03
@VermaSh
Copy link
Contributor Author

VermaSh commented Jan 29, 2025

@hzongaro I have updated the code based on your suggestions. Can I please get another review?

Copy link
Member

@hzongaro hzongaro left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

Obviously I can't fully test this in the pull request testing. You've run testing with off-heap allocation enabled, right?

@hzongaro
Copy link
Member

Jenkins test sanity.functional,sanity.openjdk xlinux,xmac,win,win32 jdk8,jdk21

@hzongaro
Copy link
Member

xmac JDK 8 sanity.functional failure appears to be due to an infrastructure problem. Rerunning.

Jenkins test sanity.functional xmac jdk8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants