Skip to content

Return geometry by value (alternative 2) - #884

Closed
SoilRos wants to merge 2 commits into
OPM:masterfrom
SoilRos:bugfix/return-geometry-by-value-2
Closed

Return geometry by value (alternative 2)#884
SoilRos wants to merge 2 commits into
OPM:masterfrom
SoilRos:bugfix/return-geometry-by-value-2

Conversation

@SoilRos

@SoilRos SoilRos commented Jun 13, 2025

Copy link
Copy Markdown
Member

To avoid copying a shared pointer, which is rather costly, the geometry now stores a variant holding either the ownership or a non-owning view to the entity variable. In the case of entity variables owned by the grid, a non-owning storage is fine as the geometry should not outlive the grid. On grid refinement the grid points are only owned by the geometry, so a move into the geometry is preferred.

Note that this PR is based on #885 which fixes all the lifetime issues of the geometry objects.

@aritorto aritorto added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Jun 13, 2025
@SoilRos
SoilRos force-pushed the bugfix/return-geometry-by-value-2 branch from b5e8c50 to 70e8f26 Compare June 13, 2025 08:50
@aritorto

Copy link
Copy Markdown
Member

jenkins build this serial please

assert(allcorners_ && cor_idx_);
return (allcorners_->data())[cor_idx_[cor]].center();
return std::visit(
Dune::overload(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just noticed that there exists Opm::VisitorOverloadSet too. Is there a reason for choosing one or the other?

@SoilRos
SoilRos force-pushed the bugfix/return-geometry-by-value-2 branch from 70e8f26 to 989459e Compare June 26, 2025 11:48
@SoilRos

SoilRos commented Jun 26, 2025

Copy link
Copy Markdown
Member Author

jenkins build this serial please

@SoilRos
SoilRos force-pushed the bugfix/return-geometry-by-value-2 branch from 989459e to 58276c4 Compare July 4, 2025 12:11
@SoilRos

SoilRos commented Jul 4, 2025

Copy link
Copy Markdown
Member Author

jenkins build this serial please

@SoilRos
SoilRos marked this pull request as draft July 4, 2025 14:02
@blattms

blattms commented Jul 9, 2025

Copy link
Copy Markdown
Member

benchmark please

@blattms

blattms commented Jul 11, 2025

Copy link
Copy Markdown
Member

Benchmark results did not get posted back here. I am on it.
The gist of the benchmark is, that there is sometimes a small increase in solution time (less than .5%). Some a runs are even faster. None really slower.

@blattms

blattms commented Jul 14, 2025

Copy link
Copy Markdown
Member

benchmark please

@blattms

blattms commented Jul 15, 2025

Copy link
Copy Markdown
Member

Manually posting results from run on July 9:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 1.002
opm-git OPM Benchmark: drogon - Threads: 8 0.885
opm-git OPM Benchmark: punqs3 - Threads: 1 0.991
opm-git OPM Benchmark: punqs3 - Threads: 8 0.982
opm-git OPM Benchmark: smeaheia - Threads: 1 0.996
opm-git OPM Benchmark: smeaheia - Threads: 8 0.923
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 0.999
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 1.007
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 1.003
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 0.934
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1 - FOPT (Total Oil Production At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8 0.909
  • Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

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

@ytelses

ytelses commented Jul 15, 2025

Copy link
Copy Markdown

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 1.007
opm-git OPM Benchmark: drogon - Threads: 8 0.899
opm-git OPM Benchmark: punqs3 - Threads: 1 1.002
opm-git OPM Benchmark: punqs3 - Threads: 8 0.991
opm-git OPM Benchmark: smeaheia - Threads: 1 1.006
opm-git OPM Benchmark: smeaheia - Threads: 8 0.922
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.004
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 0.991
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 1
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 0.898
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.998
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 0.923
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1 - FOPT (Total Oil Production At End Of Run) 1
opm-git OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8 - FOPT (Total Oil Production At End Of Run) 1
  • 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=2823

@SoilRos
SoilRos force-pushed the bugfix/return-geometry-by-value-2 branch from 58276c4 to c2c18c1 Compare July 31, 2025 14:48
@SoilRos

SoilRos commented Jul 31, 2025

Copy link
Copy Markdown
Member Author

jenkins build this serial please

@SoilRos
SoilRos force-pushed the bugfix/return-geometry-by-value-2 branch from c2c18c1 to f2e1a87 Compare August 1, 2025 10:39
@SoilRos

SoilRos commented Aug 1, 2025

Copy link
Copy Markdown
Member Author

jenkins build this serial please

SoilRos added 2 commits July 30, 2026 17:23
This is to conform with the dune interface
To avoid copying a shared pointer, which is rather costly, the geometry now stores a variant holding either the ownership or a non-owning view to the entity variable. In the case of entity variables owned by the grid, a non-owning storage is fine as the geometry should not outlive the grid. On grid refinement the grid points are only owned by the geometry, so a move into the geometry is preferred.
@SoilRos
SoilRos force-pushed the bugfix/return-geometry-by-value-2 branch from f2e1a87 to 8d6a6f4 Compare July 30, 2026 16:15
@SoilRos

SoilRos commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

jenkins build this serial please

@SoilRos

SoilRos commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Closing in favor of #1055.

@SoilRos SoilRos closed this Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual:irrelevant This PR is a minor fix and should not appear in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants