Skip to content

Revert "Removed AlignedArray (#1657)" #1838

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

Merged
merged 1 commit into from
Dec 6, 2018

Conversation

TomFinley
Copy link
Contributor

This reverts commit 72ec121.
A bug was detected that resulted in non-deterministic calculation, since the
underlying C++ code was written in a way apparently that required alignment
to produce consistent results, so of course just removing the alignment and
calling an only slightly modified algorithm compromised determinism, resulting
in test failure for RFF in particular.

If we can fix that bug by other means that would be preferable, since removing AlignedArray is a desirable outcome. Not if it means nondeterminism though, obviously. 😉

This reverts commit 72ec121.
A bug was detected that resulted in non-deterministic calculation, since the
underlying C++ code was written in a way apparently that required alignment
to produce consistent results, so of course just removing the alignment and
calling an only slightly modified algorithm compromised determinism, resulting
in test failure for RFF in particular.
@shauheen
Copy link
Contributor

shauheen commented Dec 6, 2018

Discussion in PR #1829

@Anipik
Copy link
Contributor

Anipik commented Dec 6, 2018

we may have to remove certain more things(dead code) to completely remove this change. i can put the pr if we decide that we want to continue our previous approach

@eerhardt
Copy link
Member

eerhardt commented Dec 6, 2018

Now that we've taken the "internalize all unnecessary public API" approach, I think it is much more acceptable to keep AlignedArray in ML.NET, and only use it in the limited situations where it is deemed necessary.

As long as we don't expose AlignedArray, we can remove it whenever we want in the future. So keeping it around for now (if that's what we decide makes the most sense) would be acceptable IMO.

@TomFinley
Copy link
Contributor Author

As long as we don't expose AlignedArray, we can remove it whenever we want in the future. So keeping it around for now (if that's what we decide makes the most sense) would be acceptable IMO.

Ugh. I really, really hate the AlignedArray code, even internally. Are we certain that its presence is the only way to achieve determinism?

@TomFinley TomFinley merged commit 91cee09 into dotnet:master Dec 6, 2018
@tannergooding
Copy link
Member

tannergooding commented Dec 6, 2018

Are we certain that its presence is the only way to achieve determinism

As mentioned in teams, for our SseIntrinsic and AvxIntrinsic code, it should be fairly trivial to add a new parameter that allows choosing between determinism vs perf (we don't need AlignedArray for any of this code).

However, for external dependencies that have similar optimizations, but which don't provide some switch for choosing between determnism vs perf, AlignedArray is the only thing that can guarantee determinsm.

  • MKL looks to be one such dependency, and they have notes in their documentation about needing to ensure alignment for consistent results. I haven't validated this manually yet, however.
  • Needs for best performance with Intel MKL or for reproducible results from run to run of Intel MKL functions require alignment of data arrays.

@eerhardt
Copy link
Member

eerhardt commented Dec 6, 2018

Are we certain that its presence is the only way to achieve determinism?

No, but my point is that we can ship it internally, even up to and after v1. And we can remove it at any point we'd like.

The uber point is that it isn't critical to remove it right now.

@TomFinley
Copy link
Contributor Author

The uber point is that it isn't critical to remove it right now.

Fair enough!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants