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 numberOfElements parameter to getArrayletLayout() #18268

Merged
merged 1 commit into from Oct 13, 2023

Conversation

dmitripivkine
Copy link
Contributor

0-length arrays have Discontiguous layout.
Currently detection of 0-length arrays based on dataSizeInBytes. There is not good enough for 0-stride arrays obviously (have dataSizeInBytes equal 0). All 0-stride arrays should be treated as Contiguous with exception of 0-length 0-stride should be Discontiguous. Passing of numberOfElements helps to solve this problem, dataSizeInBytes is not required to be passed any more.

0-length arrays have Discontiguous layout.
Currently detection of 0-length arrays based on dataSizeInBytes.
There is not good enough for 0-stride arrays obviously (have
dataSizeInBytes equal 0). All 0-stride arrays should be treated as
Contiguous with exception of 0-length 0-stride should be Discontiguous.
Passing of numberOfElements helps to solve this problem, dataSizeInBytes
is not required to be passed any more.

Signed-off-by: Dmitri Pivkine <Dmitri_Pivkine@ca.ibm.com>
@dmitripivkine dmitripivkine added comp:gc project:valhalla Used to track Project Valhalla related work labels Oct 11, 2023
@dmitripivkine
Copy link
Contributor Author

In #17994 fix for #17994 we have assumed 0-length 0-stride array can be treated as Contiguous (as any other 0-stride array). This appeared to be incorrect in the current code. 0-length 0-stride array has to be treated as Discontiguous exceptionally (as any other 0-length array).
There is only functional change in this PR, rest of it is code refactoring.

@amicic
Copy link
Contributor

amicic commented Oct 11, 2023

jenkins sanity aix,win jdk17

@dmitripivkine
Copy link
Contributor Author

@hangshao0 @hzongaro @a7ehuo FYI
I don't think it should change or break anything, it relates to 0-length 0-stride arrays only. The difference between Contiguous and Discontiguous arrays is an object header format in this case.

@amicic
Copy link
Contributor

amicic commented Oct 12, 2023

jenkins test sanity aix,win jdk17

@dmitripivkine
Copy link
Contributor Author

Failure in Windows build relates to infrastructure. Re-launching.

@dmitripivkine
Copy link
Contributor Author

jenkins test sanity win jdk17

@dmitripivkine
Copy link
Contributor Author

I don't see other errors in the Windows build except

18:04:33  [WS-CLEANUP] Deleting project workspace...
18:04:33  [WS-CLEANUP] Deferred wipeout is disabled by the job configuration...
18:05:02  ERROR: Cannot delete workspace :Unable to delete 'F:\Users\jenkins\workspace\Build_JDK17_x86-64_windows_Personal\openssl\NUL'. Tried 3 times (of a maximum of 3) waiting 0.1 sec between attempts.

Looks like it is reason for failure for me.

@amicic amicic merged commit 2645298 into eclipse-openj9:master Oct 13, 2023
4 of 6 checks passed
@hzongaro
Copy link
Member

@dmitripivkine @amicic, would this change have caused this error in the Windows PR builds:

18:00:28  f:\users\jenkins\workspace\build_jdk17_x86-64_windows_personal\openj9\runtime\gc_glue_java\ArrayletObjectModel.hpp(1152): error C2220: the following warning is treated as an error
18:00:28  f:\users\jenkins\workspace\build_jdk17_x86-64_windows_personal\openj9\runtime\gc_glue_java\ArrayletObjectModel.hpp(1152): warning C4244: '=': conversion from 'uintptr_t' to 'uint32_t', possible loss of data

@amicic
Copy link
Contributor

amicic commented Oct 13, 2023

it indeed is (ArrayletObjectModel.hpp(1152)). I'll revert it - it's not an urgent change, anyway

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:gc project:valhalla Used to track Project Valhalla related work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants