Skip to content

Return geometry by value (alternative 4) - #1055

Open
SoilRos wants to merge 3 commits into
OPM:masterfrom
SoilRos:bugfix/return-geometry-by-value-4
Open

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

Conversation

@SoilRos

@SoilRos SoilRos commented Jul 30, 2026

Copy link
Copy Markdown
Member

This is yet another version of how to return the geometry by value (see #885, #884, #891, and #334). Compared to the others, this should be the fastest one. The trick is made by not sharing the ownership of the corners, just a non-owning pointer. Additionally, since the geometryInFather needs to store its own data, I simply changed its type for Dune::AxisAlignedCubeGeometry which does the same job, but faster.

SoilRos added 3 commits July 30, 2026 20:59
@SoilRos
Use Dune::AxisAlignedCubeGeometry for entity local geometries
81162fb 
This change switches the `LocalGeometry` type and the return type of `geometryInFather()` to `Dune::AxisAlignedCubeGeometry`. This simplifies the implementation of `geometryInFather()` and leverages a standard Dune geometry type for axis-aligned refined cells.

The local geometry is a mapping from one cube reference element to another cube reference element. For this, there is no need to store all corners, just the upper-left and lower-bottom corners.
@SoilRos
Return geometry objects by value
649ab52 
This is to conform with the dune interface
@SoilRos
Refactor Geometry to use non-owning pointers for corner data
fe28d09 
The `Geometry` class previously used `std::shared_ptr` to manage its corner geometry data. This commit changes the `allcorners_` member to a raw pointer, making `Geometry` a non-owning view of this data.

This refactoring aligns with the goal of `Geometry` objects being lightweight value types, enabling them to be returned by value more efficiently and consistently with the Dune interface. A warning has been added to the constructor to explicitly state these new ownership semantics.
@SoilRos SoilRos added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Jul 30, 2026
@SoilRos

SoilRos commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

jenkins build this serial please

@SoilRos

SoilRos commented Jul 30, 2026

Copy link
Copy Markdown
Member Author

benchmark please

@ytelses

ytelses commented Jul 31, 2026

Copy link
Copy Markdown

Benchmark result overview:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 0.998
opm-git OPM Benchmark: drogon - Threads: 8 1.015
opm-git OPM Benchmark: punqs3 - Threads: 1 0.991
opm-git OPM Benchmark: punqs3 - Threads: 8 0.998
opm-git OPM Benchmark: smeaheia - Threads: 1 1.003
opm-git OPM Benchmark: smeaheia - Threads: 8 0.987
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.008
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 0.994
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 1.001
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 0.992
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 0.992
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 1.003
  • 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=3163

@SoilRos

SoilRos commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

The table above seems wrong, the actual table reported in the website is:

Test Configuration Relative
opm-git OPM Benchmark: drogon - Threads: 1 1.001
opm-git OPM Benchmark: drogon - Threads: 8 1.212
opm-git OPM Benchmark: punqs3 - Threads: 1 0.996
opm-git OPM Benchmark: punqs3 - Threads: 8 1.012
opm-git OPM Benchmark: smeaheia - Threads: 1 0.998
opm-git OPM Benchmark: smeaheia - Threads: 8 1.075
opm-git OPM Benchmark: spe10_model_1 - Threads: 1 1.009
opm-git OPM Benchmark: spe10_model_1 - Threads: 8 0.995
opm-git OPM Benchmark: flow_mpi_extra - Threads: 1 1.002
opm-git OPM Benchmark: flow_mpi_extra - Threads: 8 1.13
opm-git OPM Benchmark: flow_mpi_norne - Threads: 1 1.002
opm-git OPM Benchmark: flow_mpi_norne - Threads: 8 1.078
  • Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

@SoilRos

SoilRos commented Jul 31, 2026

Copy link
Copy Markdown
Member Author

Locally (Apple M3 Max), I get simular results:

Screenshot 2026-07-31 at 14 43 37

To me, this suggests that the assembly is already copying the geometry somewhere and we have been paying the price of the shared pointer reference counting.

This was referenced Jul 31, 2026
Closed
Closed
Closed
@atgeirr

atgeirr commented Aug 3, 2026

Copy link
Copy Markdown
Member

Just a question: how can Dune::AxisAlignedCubeGeometry be used, our grid cells are not axis aligned (except for artificial test cases)?

@SoilRos

SoilRos commented Aug 12, 2026

Copy link
Copy Markdown
Member Author

@atgeirr geometryInFather() transforms the reference element of the child element onto the reference element of the father element. Both are axial aligned cube geometries even if the original element of the father/child element is a strange geometry, even if several corners are collapsed into one.

@SoilRos
SoilRos requested a review from atgeirr August 20, 2026 11:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@atgeirr atgeirr Awaiting requested review from atgeirr

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SoilRos @ytelses @atgeirr