Disable fused multiply-subtract optimizations on activator calculations #8341
Conversation
Fixes TestAutoscalerPanicModeExponentialTrackAndStablize on arm64, ppc64le and s390x. While we are here make the order of operations match between the expectedECB and excessBCF calculations. Floating point operations aren't associative so performing them in the same order reduces the risk of unexpected differences. Background: On some platforms (not amd64 - unless using a compiler other than gc) the Go compiler 'fuses' some floating point operations. This improves performance and sometimes accuracy. However this can cause issues with test data that has been generated on a platform without such fused operations (such as amd64) or has been constant folded. A simplified example of the problem (values changed): Without fused multiply-subtract: 1.333... * 3 - 2 = 2.000... With fused multiply-subtract: 1.333... * 3 - 2 = 1.999... This commit disables fused multiply-subtract operations in all excess burst capacity calculations. This ensures that the calculations match across all platforms and in situations where constant folding takes place.
knative-prow-robot
added
the
size/S
|
Welcome @mundaym! It looks like this is your first PR to knative/serving 🎉 |
|
Hi @mundaym. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
knative-prow-robot
added
the
needs-ok-to-test
|
/assign @vagababov |
|
/ok-to-test |
knative-prow-robot
added
ok-to-test
|
Since I am unaware of platform specifics and I am curious :), would you care to explain what is not working on those platforms? |
|
I can deal with approximations in the tests. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mundaym, vagababov The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow-robot
added
the
approved
Fixes TestAutoscalerPanicModeExponentialTrackAndStablize on arm64, ppc64le and s390x. These platforms aren't currently supported by Knative but it can be built from source on them and fixing this test will help any future porting efforts smoother. This is especially true since it is the only unit test that appears to fail on these platforms.
This change won't affect amd64 at all, except when using a compiler that supports fused multiply-subtract operations (e.g. gccgo or gollvm with -Ofast).
Ideally we'd make the tests and/or calculations more tolerant of minor variations in the output of individual floating point operations. Here the test failure is caused by a tiny numerical difference that is magnified by rounding down:
math.Floor(330.999...)vs.math.Floor(331.000...). I'm not certain of the best way to do that though and it would certainly be a much larger and more invasive change.Thanks for taking a look!