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

Improve the conversions benchmarks #317

Merged
merged 1 commit into from
Jun 24, 2019
Merged

Improve the conversions benchmarks #317

merged 1 commit into from
Jun 24, 2019

Conversation

tcojean
Copy link
Member

@tcojean tcojean commented Jun 19, 2019

This PR adds some performance and result quality improvement to the
conversions benchmarks.

  • Move the construction of matrix_to in convert_matrix function. This allows
    to catch the related exceptions in a way similar to the other benchmarks.
  • Do not construct the matrix_to object until the use of copy_from in the
    warmup. This allows to catch non-existing conversions quickly (e.g.
    ELL->COO), without the overhead of loading big matrix data first.
  • Catch the AllocationError related exceptions for matrix_from and add a
    completed: false entry to the results on failure to instantiate a matrix
    from the data. This happens a lot when converting from ELL for example,
    which can take a lot of space.

+ Move the construction of `matrix_to` in `convert_matrix` function. This allows
	to catch the related exceptions.
+ Do not construct the `matrix_to` object until the use of `copy_from` in the
	warmup. This allows to catch non-existing conversions quickly, without the
	overhead of loading big matrix data first.
+ Catch the `AllocationError` related exceptions for `matrix_from` and add a
	`completed: false` entry to the results on failure to instantiate a matrix
	from the data.
@tcojean tcojean self-assigned this Jun 19, 2019
@tcojean tcojean added reg:benchmarking This is related to benchmarking. is:enhancement An improvement of an existing feature. 1:ST:ready-for-review This PR is ready for review labels Jun 19, 2019
Copy link
Collaborator

@hartwiganzt hartwiganzt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov
Copy link

codecov bot commented Jun 19, 2019

Codecov Report

Merging #317 into develop will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #317      +/-   ##
===========================================
- Coverage    98.21%   98.17%   -0.05%     
===========================================
  Files          224      223       -1     
  Lines        17321    17180     -141     
===========================================
- Hits         17012    16866     -146     
- Misses         309      314       +5
Impacted Files Coverage Δ
omp/factorization/par_ilu_kernels.cpp 0% <0%> (-100%) ⬇️
core/base/mtx_io.cpp 92.85% <0%> (-0.65%) ⬇️
omp/test/factorization/par_ilu_kernels.cpp

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 c4f567e...02f393b. Read the comment docs.

Copy link
Member

@thoasm thoasm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tcojean tcojean merged commit ec7918f into develop Jun 24, 2019
@tcojean tcojean deleted the improve_conversions branch June 24, 2019 08:54
@tcojean tcojean added 1:ST:ready-to-merge This PR is ready to merge. and removed 1:ST:ready-for-review This PR is ready for review labels Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-to-merge This PR is ready to merge. is:enhancement An improvement of an existing feature. reg:benchmarking This is related to benchmarking.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants