-
Notifications
You must be signed in to change notification settings - Fork 995
Improve EEST mod_vul_guido_3_even use case #9158
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
Improve EEST mod_vul_guido_3_even use case #9158
Conversation
Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net>
siladu
left a comment
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.
From your table it's not clear to me what the actual improvement is.
For each test case, I am not sure whether the current behaviour will use native or Java, following @lu-pinto's original optimisation.
Also, why are the mod and base lengths significant for this PR?
| final int modulusLength = clampedToInt(length_of_MODULUS); | ||
| if ((extractLastByte(input, baseOffset, baseLength) & 1) != 1 | ||
| && (extractLastByte(input, modulusOffset, modulusLength) & 1) != 1) { | ||
| if ((extractLastByte(input, baseOffset, baseLength) & 1) != 1) { |
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 double check my understanding: this PR changes the code from:
"If the least significant byte of both the base and the modulus is even, then use native"
to
"If the least significant byte of the base is even, then use native"
...so we're using native in more cases than we were before?
How does that relate to the underlying algorithm? ...I guess I'm wondering why the extra restriction for even modulus was originally added cc @lu-pinto ?
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.
Your understanding is correct. Let’s confirm with @lu-pinto, but from what I gathered, he seems to agree with this implementation.
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.
This was to avoid going through the other heavy branch full of BigInteger operations that generate confetti objects: https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/math/BigInteger.java#L3003-L3004
Odd modulus makes code go through the Montegomery algorithm, so I thought it would make a difference. Apparently this case invalidates that assumption somehow. I haven't studied it as thoroughly, but this change seems to normalize results which is good for the Ethereum case.
There could be a better routing strategy, or just code up a Java impl that gets rid of all the new objects - this would be my preferred approach because JNA has a cost.
It is in the description but I agree that it is not very clear "The last column in the table below shows the improvement from the PR".
I was looking for heuristics based on the input of new found use case. I found from the benchmarks that native implementation is better for that specific use case, so I tested 3 strategies. The last one is the best as it keeps better mgas/s on other use cases. |
lu-pinto
left a comment
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.
I would say let's ship this and reiterate after
I saw that, but what's not clear is what the "before" value is, since both native and java values are listed rather than the one that the benchmark actually uses based on the input... |
siladu
left a comment
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.
Confirmed with @ahamlat that "current with JAVA default (MGps)" column is comparable with the last column "if base even execute native" since both are switching between Java and native based on input.
|
I think you can forget these columns "if mod length > 512 then native" and "if base length > 256 then native". These are attempts that are not implemented in this PR as they have some regressions
There're only two implementation of ModExp, either Java, either Native. We use the heuristics to decice which one to execute. Currently in besu, there is no way to force all ModExp executions to JAVA. So even in the first columns, some executions are done with native implementation because the existing heuristics. |
* Improve EEST mod_vul_guido_3_even use case Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> Signed-off-by: georgereuben <reubengeorge101@gmail.com>
* Improve EEST mod_vul_guido_3_even use case Signed-off-by: Ameziane H. <ameziane.hamlat@consensys.net> Signed-off-by: jflo <justin+github@florentine.us>
PR description
This PR goal is to improve the performances of a modExp worst case scenario from EEST tests. The input of that specific use case was added to Mod exp benchmarks. The last column in the table below shows the improvement from the PR.
Fixed Issue(s)
Thanks for sending a pull request! Have you done the following?
doc-change-requiredlabel to this PR if updates are required.Locally, you can run these tests to catch failures early:
./gradlew spotlessApply./gradlew build./gradlew acceptanceTest./gradlew integrationTest./gradlew ethereum:referenceTests:referenceTests