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

GasPvtMultiplexer: Replace unholy trinity with visitor overload sets #3279

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

Conversation

akva2
Copy link
Member

@akva2 akva2 commented Dec 14, 2022

Sits on top of #3278
Waiting for #3246

I realize this may be controversial, and that it might have runtime implications (which we need to benchmark) but;
This replaces what I consider the unholy trinity (void pointers, macros and SFINAE) with the visitor overload set idiom in the gas multiplexer.

As a bonus we avoid the need to have explicit ctors, copy ctors, assignment and comparison operators.

@bska
Copy link
Member

bska commented Dec 14, 2022

As a bonus we avoid the need to have explicit ctors, copy ctors, assignment and comparison operators.

🎉

@akva2
Copy link
Member Author

akva2 commented Dec 23, 2022

jenkins build this opm-models=767 opm-simulators=4325 please

@akva2
Copy link
Member Author

akva2 commented Dec 23, 2022

benchmark please opm-models=767 opm-simulators=4325

@ytelses
Copy link

ytelses commented Dec 23, 2022

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 1.026
opm-git OPM Benchmark: drogon - Threads: 8 0.989
opm-git OPM Benchmark: smeaheia - Threads: 1 1.02
opm-git OPM Benchmark: smeaheia - Threads: 8 0.906
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 1.001
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 0.901
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.995
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 0.983
  • 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=1899

@akva2
Copy link
Member Author

akva2 commented Jan 4, 2023

benchmark please opm-models=767 opm-simulators=4325

@ytelses
Copy link

ytelses commented Jan 4, 2023

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 0.989
opm-git OPM Benchmark: drogon - Threads: 8 0.982
opm-git OPM Benchmark: smeaheia - Threads: 1 1.006
opm-git OPM Benchmark: smeaheia - Threads: 8 0.992
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 0.987
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 0.983
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.989
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 0.976
  • 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=1912

@akva2
Copy link
Member Author

akva2 commented Mar 8, 2023

I did my own benchmarking, and sadly it seems the variant approach is significantly slower (this is with the whole PR chain);

Total time (10 samples) for NORNE_ATW2013

procs orig variant
1 520.49 +- 3.53 551.50 +- 1.57 (p=0.00)
8 90.77 +- 0.34 99.45 +- 0.44 (p=0.00)

@bska
Copy link
Member

bska commented Mar 8, 2023

sadly it seems the variant approach is significantly slower

That's disappointing.

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

3 participants