The Version table provides details related to the release that this issue/RFE will be addressed.
Unresolved : Release in which this issue/RFE will be addressed. Resolved: Release in which this issue/RFE has been resolved. Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.
Accelerate CRC32-C on x86 architecture (intrinsic).
Comments
Thanks Scott, with that reasoning I understand that the risk is acceptable.
20-09-2022
As far as a risk assessment, I've looked at the changes and found that they're mostly parameter name changes, which adds to the readability. I see no appreciable risk due to the additional code and table that has been introduced, and there are no errors flagged with the testing that has been done.
Granted, this does introduce minimal risk (IMO) to the CRC32-C algorithm, but I don't see why classifying this as high risk is warranted. Any change to any algorithm introduces risk, regardless if it's in assembly (which we're really good at, BTW) or C or Java. We do our best to thoroughly review and test changes we make. There is no attempt to minimize or hide any risk introduced as the result of this change. This is a backport of the identical functionality working in JDK-18 which in my opinion is working as advertised.
Please let me know what specific risk you see so we can better learn what is expected. I can't quantify statements like "you break many installations if you introduce an error." This statement is true always, regardless of the scope of a change.
19-09-2022
Hi Goetz,
Thanks a lot for considering the backport request and running the change through testing. I understand your point of view now that x86 has a large user base. My risk statement was solely based on the fact that this change is limited to CRC32C.
There seems to be an issue with Github actions for my account and I have reached out to Github support.
19-09-2022
Hi,
I am critisizing your risk assesment. With mentioning that x86 is an important platform
I wanted to make the point that you break many installations if you introduce an error,
so this makes the risk high.
Your answer 1) gives the reason why you want to backport the change, but does not
touch the risk assessment. 2) also is not a risk assessment.
In future, please be careful with your risk assesment. Do not mix it with
other points, and don't underestimate the risk. To me this looks as if
you want to hide the risk to increase the changes of approval.
Thanks.
I will run the change through our testing. And please enable Pre-submit testing in your github repo.
19-09-2022
Hi Goetz,
Thank you for your comment.
1) Yes you are correct that x86-64 platform is important. However, it is also important for us at Intel that CRC32C reaches customers as part of JDK17 release.
2) This algorithm is part of ISA-L. It is based on CRC32 algorithm with change to the table.
3) It was originally introduced in JDK18, so it has been undergoing testing.
Do let me know of your thoughts. Thank you.
16-09-2022
Hi,
I think the risk is HIGH "as it affects CRC32C functionality for x86-64 platforms".
* x86_64 is the most important platform
* CRC32C is an important algorithm
* Developing these algorithms in assembly is error-prone.
The point that might qualify this is that the change is well hung.
16-09-2022
A pull request was submitted for review.
URL: https://git.openjdk.org/jdk17u-dev/pull/670
Date: 2022-09-15 00:26:26 +0000
15-09-2022
Fix Request 17u
This patch shows significant performance gain; for example 1.75x for 16k buffer size. The patch applies cleanly to jdk17u-dev. The patch is low risk as it affects CRC32C functionality for x86-64 platforms. Testing conducted: tier-1.
15-09-2022
Hi [~sgibbons], when comparing to other CPU [ppc64,s390x] platforms, I wonder should there be additional c1 coding for this ?
src/hotspot/cpu/x86/c1_LIRGenerator_x86.cpp
1099 void LIRGenerator::do_update_CRC32C(Intrinsic* x) {
1100 Unimplemented();
1101 }
and
src/hotspot/share/c1/c1_Compiler.cpp
217 #if defined(S390) || defined(PPC64) || defined(AARCH64)
218 case vmIntrinsics::_updateBytesCRC32C:
219 case vmIntrinsics::_updateDirectByteBufferCRC32C:
220 #endif
Thanks for the heads-up. I just proposed a PR that satisfies this RFE. Hopefully I can get it integrated soon.
29-11-2021
Hi [~sgibbons], just to give you a heads-up, if you plan to get this into 18 it needs to be integrated before the fork is coming up early in December. Otherwise, it must be deferred to 19 (as it is an RFE).