Skip to content
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

Uninitialize working memory #243

Merged
merged 3 commits into from
Aug 2, 2020
Merged

Uninitialize working memory #243

merged 3 commits into from
Aug 2, 2020

Conversation

termoshtt
Copy link
Member

@termoshtt termoshtt commented Aug 1, 2020

Split from #241

Current implementation uses zero-initialization like

let mut work = vec![Self::zero(); n];

This will initialize work memory using memset, which would be fastest to initialize memory. This memory initialization make our code safe for accessing like work[i], however, this is relatively heavy comparing to uninitialized code:

let mut work = Vec::with_capacity(n);
unsafe { work.set_len(n) };

This takes O(1) time while initialization needs O(n) time, but access work[i] causes undefined behavior even if i < n.
Because LAPACK routines are assured not to read work memory, this uninitialized version can be used.

This cost is usually neglectable comparing to most LAPACK routines, which often takes O(n^2) or O(n^3) for large n. But if we focus on the memory allocation cost #241 #168, it should be considered, and split them to evaluate memory allocation cost properly.

Alternative

let work = vec![std::mem::MaybeUninit::uninit(); n];

could be another way. It actually does not initialize the memory, and almost same performance as above uninitialized version. I do not use it on this PR because some API, especially slice_get_mut is not stabilized yet. This will be better choice for future rustc where they are stabilized.

Links

@codecov
Copy link

codecov bot commented Aug 1, 2020

Codecov Report

Merging #243 into master will increase coverage by 0.01%.
The diff coverage is 93.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
+ Coverage   89.07%   89.08%   +0.01%     
==========================================
  Files          71       71              
  Lines        3588     3592       +4     
==========================================
+ Hits         3196     3200       +4     
  Misses        392      392              
Impacted Files Coverage Δ
lax/src/solveh.rs 78.78% <66.66%> (ø)
lax/src/eig.rs 89.00% <81.81%> (ø)
lax/src/eigh.rs 94.44% <83.33%> (ø)
lax/src/least_squares.rs 100.00% <100.00%> (ø)
lax/src/lib.rs 100.00% <100.00%> (ø)
lax/src/opnorm.rs 100.00% <100.00%> (ø)
lax/src/qr.rs 100.00% <100.00%> (ø)
lax/src/rcond.rs 100.00% <100.00%> (ø)
lax/src/solve.rs 100.00% <100.00%> (ø)
lax/src/svd.rs 100.00% <100.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73d4e77...dc2ab7d. Read the comment docs.

@termoshtt termoshtt added this to the 0.13.0 milestone Aug 1, 2020
@termoshtt
Copy link
Member Author

Needs benchmark code

@termoshtt termoshtt mentioned this pull request Aug 1, 2020
13 tasks
@termoshtt termoshtt marked this pull request as ready for review August 2, 2020 14:00
@termoshtt
Copy link
Member Author

I have confirmed this PR have small (~0.1 us, ~10%) performance gain for svddc/C/4 benchmark by #244, though they cannot split from noise for larger matrices.
image

@termoshtt termoshtt merged commit 6ab2b48 into master Aug 2, 2020
@termoshtt termoshtt deleted the vec_uninit branch August 2, 2020 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant