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.

Sign up for GitHub

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

Jump to bottom

Ghost entries skipped for ILU apply and SpMV operator in all levels of AMG/CPR hierarchy #5182

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ghost entries skipped for ILU apply and SpMV operator in all levels of AMG/CPR hierarchy #5182

wants to merge 2 commits into from

Conversation

lisajulia
Copy link
Contributor

Replacement for #4296

@lisajulia
Copy link
Contributor Author

jenkins build this please

@lisajulia
Copy link
Contributor Author

jenkins build this please

@blattms
Copy link
Member

blattms commented Feb 12, 2024

Fortunately, no time stepping changes and hence failed tests. Just new warnings that need to be removed.

@lisajulia
Copy link
Contributor Author

jenkins build this please

@lisajulia
Copy link
Contributor Author

jenkins build this please

@lisajulia
Copy link
Contributor Author

benchmark please

@lisajulia
Copy link
Contributor Author

@blattms: I believe the last comment "benchmark please" did not have any effect - can you show me where I can check that?

@lisajulia
Copy link
Contributor Author

jenkins build this please

@blattms
Copy link
Member

blattms commented Feb 12, 2024

turns out you can't. I asked Michael to whitelist you and @aritorto. It is "benchmark please" and should be used scarcely. We cannot really see whether it worked. Normally the benchmarking report is added to the PR after a few hours, but currently that is broken, too.

@blattms
Copy link
Member

blattms commented Feb 12, 2024

benchmark please

@ytelses
Copy link

ytelses commented Feb 13, 2024

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 0.999
opm-git OPM Benchmark: drogon - Threads: 8 0.997
opm-git OPM Benchmark: punqs3 - Threads: 1 1.007
opm-git OPM Benchmark: punqs3 - Threads: 8 1.003
opm-git OPM Benchmark: smeaheia - Threads: 1 0.965
opm-git OPM Benchmark: smeaheia - Threads: 8 1
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.013
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 1.004
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 - FOIT (Total Oil Injection At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 - FOIT (Total Oil Injection At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.991
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 1.006
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1 1.008
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8 0.998
  • Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2370

@ytelses
Copy link

ytelses commented Feb 13, 2024

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 1.005
opm-git OPM Benchmark: drogon - Threads: 8 0.816
opm-git OPM Benchmark: punqs3 - Threads: 1 1.002
opm-git OPM Benchmark: punqs3 - Threads: 8 0.988
opm-git OPM Benchmark: smeaheia - Threads: 1 0.955
opm-git OPM Benchmark: smeaheia - Threads: 8 0.885
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.008
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 0.998
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 - FOIT (Total Oil Injection At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 - FOIT (Total Oil Injection At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.99
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 0.956
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1 1.001
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8 0.931
  • Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2371

@ytelses
Copy link

ytelses commented Feb 13, 2024

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 1.003
opm-git OPM Benchmark: drogon - Threads: 8 0.992
opm-git OPM Benchmark: punqs3 - Threads: 1 0.991
opm-git OPM Benchmark: punqs3 - Threads: 8 1.012
opm-git OPM Benchmark: smeaheia - Threads: 1 0.969
opm-git OPM Benchmark: smeaheia - Threads: 8 1.001
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.002
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 1.001
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 - FOIT (Total Oil Injection At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 - FOIT (Total Oil Injection At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.995
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 1.001
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1 0.995
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8 0.993
  • Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2372

@ytelses
Copy link

ytelses commented Feb 14, 2024

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 0.998
opm-git OPM Benchmark: drogon - Threads: 8 0.992
opm-git OPM Benchmark: punqs3 - Threads: 1 0.989
opm-git OPM Benchmark: punqs3 - Threads: 8 1.009
opm-git OPM Benchmark: smeaheia - Threads: 1 0.969
opm-git OPM Benchmark: smeaheia - Threads: 8 1.001
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.019
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 1.005
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 - FOIT (Total Oil Injection At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 - FOIT (Total Oil Injection At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 1.006
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 1.005
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1 0.999
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8 0.993
  • Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2373

@atgeirr
Copy link
Member

atgeirr commented Feb 14, 2024

Looks like no change in the benchmarks, which is as expected, since they all run using the default linear solver (i.e. ILU0 preconditioner, no CPR/AMG) I believe.

@lisajulia
Copy link
Contributor Author

Looks like no change in the benchmarks, which is as expected, since they all run using the default linear solver (i.e. ILU0 preconditioner, no CPR/AMG) I believe.

Yes that makes sense. Curretly, I'm still waiting for Andreas to get back to me with the measurements he has done previously.

@lisajulia lisajulia force-pushed the ilu-op-in-amg branch 2 times, most recently from dddb44e to 3818c75 Compare March 15, 2024 07:37
@lisajulia
Copy link
Contributor Author

lisajulia commented Mar 15, 2024 •
edited by bska

Looks like no change in the benchmarks, which is as expected, since they all run using the default linear solver (i.e. ILU0 preconditioner, no CPR/AMG) I believe.

Yes that makes sense. Curretly, I'm still waiting for Andreas to get back to me with the measurements he has done previously.

New results using the current master (N = number of processes, simulation time in seconds for normal and skipping ghost) below, for 32 and 64 processes the improvement is still around 5% and for 128 processes, the improvement is still around 10%!

N normal skip ghost
32 1043.34 1006.81
64 820.60 773.49
128 589.27 541.67
256 still running still running

blattms
blattms reviewed Mar 15, 2024
View reviewed changes
Copy link
Member

@blattms blattms left a comment

Choose a reason for hiding this comment

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

Review is not complete yet.

Looking at this PR I suddenly realize that we are already assuming that copy/overlap entries come after owner. The certainly holds when using CpGrid, but I kind of doubt that it holds for other parallel grids 😬 . It seems like there is no check for this in master anywhere, but I'll double check this.

Anyway, if this assumption is true then this restriction has been there for quite some time without us noticing. Hence it should not hold merging back.

tests/test_ghostlastmatrixadapter.cpp
Comment on lines +85 to +86
for(int j=0; j < N; j++)
for(int i=overlapStart; i < overlapEnd; i++, ++iter) {
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to change this, to make the sure that the guys that are GridAttributes::copy come after the rest?

I think we would need some kind of mapping of the indices to ensure that?

Not sure whether we want an expensive check in GhostLastMatrxAdapter that checks the GridAttributes::owner comes first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we discuss this tomorrow once more and then eventually merge?

@lisajulia
Copy link
Contributor Author

jenkins build this please

@lisajulia
Copy link
Contributor Author

jenkins build this please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atgeirr atgeirr atgeirr left review comments

@blattms blattms blattms left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@lisajulia @blattms @ytelses @atgeirr @andrthu