-
Notifications
You must be signed in to change notification settings - Fork 737
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
Remove flattening flag from nullable arrays #20250
Conversation
56d07c6
to
0289d22
Compare
@a7ehuo can you review the tests on this change? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test update looks good to me. Thank you very much for the change @theresa-m!
38e1116
to
58cb366
Compare
58cb366
to
0e1b00f
Compare
0e1b00f
to
3ce1129
Compare
test/functional/Valhalla/src_qtypes/org/openj9/test/lworld/ValueTypeSystemArraycopyTests.java
Show resolved
Hide resolved
Update ValueTypeUnsafeTests tests to use flattenable array. Update ValueTypeSystemArraycopyTests to test System.arrayCopy between nullable and null-restricted arrays. Signed-off-by: Theresa Mammarella <[email protected]>
3ce1129
to
533dc85
Compare
d45e43f
to
d2eab92
Compare
test/functional/Valhalla/src_qtypes/org/openj9/test/lworld/ValueTypeSystemArraycopyTests.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much for the update! Sorry I just realized I missed a few things in my first review. They are minor comments
nullRestrictedVtArrayDst[i] = new SomeValueClass(i*5); | ||
nullRestrictedVtArraySrc[i] = new SomeValueClass(i*6); | ||
|
||
ifIdArrayDst[i] = new SomeIdentityClass(i*7); | ||
ifIdArraySrc[i] = new SomeIdentityClass(i*8); | ||
|
||
ifVtArrayDst[i] = new SomeValueClass(i*9); | ||
ifVtArraySrc[i] = new SomeValueClass(i*10); | ||
|
||
ifPrimitiveVtArrayDst[i] = new SomePrimitiveValueClass(i*11); | ||
ifPrimitiveVtArraySrc[i] = new SomePrimitiveValueClass(i*12); | ||
ifNullRestrictedVtArrayDst[i] = new SomeValueClass(i*11); | ||
ifNullRestrictedVtArraySrc[i] = new SomeValueClass(i*12); | ||
|
||
ifArray1[i] = new SomeIdentityClass(i*13); | ||
ifArray2[i] = new SomeValueClass(i*14); | ||
ifArray3[i] = new SomePrimitiveValueClass(i*15); | ||
ifArray3[i] = new SomeValueClass(i*15); | ||
|
||
nullRestrictedVtArrayDst[i] = new SomeValueClass(i*16); | ||
nullRestrictedVtArraySrc[i] = new SomeValueClass(i*17); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nullRestrictedVtArrayDst[i]
and nullRestrictedVtArraySrc[i]
are now being initialized twice: line 124/125 and line 140/141. I think only one set of initialization is required
initArrays(); | ||
testVTVT(vtArraySrc, nullRestrictedVtArrayDst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To properly test the interpreter and the JIT, these two lines need to run twice since a lot of JIT options include count=1
. Running it once won't get testVTVT
method compiled. And the result should be checked to make sure the array element data is copied correctly.
Maybe something like below
@Test(priority=1)
static public void testSystemArrayCopy27() throws Throwable {
initArrays();
testVTVT(vtArraySrc, nullRestrictedVtArrayDst);
checkResults(vtArraySrc, nullRestrictedVtArrayDst);
initArrays();
testVTVT(vtArraySrc, nullRestrictedVtArrayDst);
checkResults(vtArraySrc, nullRestrictedVtArrayDst);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use the attribute @Test(priority=1,invocationCount=2)
, if the test has the -Xjit:count=1,disableAsyncCompilation
option specified.
initArrays(); | ||
testVTVT(nullRestrictedVtArraySrc, vtArrayDst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comments as the above
@Test(priority=1)
static public void testSystemArrayCopy28() throws Throwable {
initArrays();
testVTVT(nullRestrictedVtArraySrc, vtArrayDst);
checkResults(nullRestrictedVtArraySrc, vtArrayDst);
initArrays();
testVTVT(nullRestrictedVtArraySrc, vtArrayDst);
checkResults(nullRestrictedVtArraySrc, vtArrayDst);
}
initArraysToCopyNullToNullRestrictedArray(); | ||
testVTVT(vtArraySrc, nullRestrictedVtArrayDst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To properly test the interpreter and the JIT, these two lines need to run twice. Maybe something like below
@Test(priority=1)
static public void testSystemArrayCopy29() throws Throwable {
try {
initArraysToCopyNullToNullRestrictedArray();
testVTVT(vtArraySrc, nullRestrictedVtArrayDst); // First time invoke the interpreter
} catch (java.lang.ArrayStoreException ase1) {
try {
initArraysToCopyNullToNullRestrictedArray();
testVTVT(vtArraySrc, nullRestrictedVtArrayDst); // Second time JIT'd method
} catch (java.lang.ArrayStoreException ase2) {
// pass
return;
}
}
Assert.fail("Expect a ArrayStoreException. No exception or wrong kind of exception thrown");
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or we sometimes adjust the @Test
attribute to run the test method twice, so methods will be compiled the second time if the test is accompanied by the -Xjit:count=1,disableAsyncCompilation
option.
@Test(priority=1,invocationCount=2)
d2eab92
to
db40903
Compare
checkResults(vtArraySrc, nullRestrictedVtArrayDst); | ||
|
||
initArrays(); | ||
testVTVT(vtArraySrc, nullRestrictedVtArrayDst); | ||
checkResults(vtArraySrc, nullRestrictedVtArrayDst); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use Henry's suggestion, #20250 (comment) instead of copying/paste the code twice like I initially suggested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment applies to testSystemArrayCopy28 and testSystemArrayCopy29 as well
Signed-off-by: Theresa Mammarella <[email protected]>
db40903
to
1f11001
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the update on the test file!
Jenkins test sanity,extended zlinuxval jdknext |
Jenkins compile amac jdk23 |
The sanity.openjdk failure is not related to this change. |
Update ValueTypeUnsafeTests tests to use flattenable array.
Update ValueTypeSystemArraycopyTests to test System.arrayCopy
between nullable and null-restricted arrays.
Fixes: #20223
Fixes: #20253