Refactor: BMI adapters' virtual destructor hierarchy and general cleanup #750

program-- · 2024-02-29T16:16:18Z

This PR resolves #747 by pulling the virtual destructor hierarchy one level above AbstractCLibBmiAdapter, so that Bmi_Adapter defines a virtual destructor.

Moreover, this PR includes some additional cleanup of the BMI adapter classes and supporting files.

Additions

Adds a default virtual destructor to Bmi_Adapter.
Adds an overridden destructor to Bmi_Py_Adapter.
Adds source files for AbstractCLibBmiAdapter and Bmi_Adapter.

Changes

Reworks Bmi listfile to remove call to dynamically sourced library, and instead manually creates the CMake target to handle conditional compilation properly.
Replaces virtual destructor attribute with override in subclasses of Bmi_Adapter and AbstractCLibBmiAdapter.
Updates and sorts include directives in header and source files.

Checklist

Target Environment support

Linux
macOS

program--

Added some review comments to help guide review and clarify design decisions. 😄

include/bmi/AbstractCLibBmiAdapter.hpp

program-- · 2024-02-29T18:41:28Z

include/bmi/AbstractCLibBmiAdapter.hpp


        private:

            /** Path to the BMI shared library file, for dynamic linking. */
            std::string bmi_lib_file;
            /** Name of the function that registers BMI struct's function pointers to the right module functions. */
-            const std::string bmi_registration_function;
+            std::string bmi_registration_function;


Removed const specifier, since it isn't necessary here. Really, the getter for this should be const instead.

Agree the getter could be marked const, but that would prevent future code added to the class from trying to change the bmi_registration_function, perhaps even inadvertently, which could lead to unexpected behavior. I guess I'm trying to say, it doesn't hurt to be overly pedantic in these cases where we want the compiler to help with validation as much as possible.

Reverted in 4f83059

include/bmi/Bmi_Adapter.hpp

include/bmi/Bmi_Py_Adapter.hpp

src/bmi/AbstractCLibBmiAdapter.cpp

program-- · 2024-02-29T18:47:16Z

src/bmi/CMakeLists.txt

-include(${PROJECT_SOURCE_DIR}/cmake/dynamic_sourced_library.cmake)
-dynamic_sourced_cxx_library(ngen_bmi "${CMAKE_CURRENT_SOURCE_DIR}")


Dynamic sourcing is being removed because we should aim to eventually get rid of it completely. Conditional compilation is better managed by the build system rather than in the C++ files themselves, but this dynamic sourcing is preventing that from happening.

program-- · 2024-02-29T18:52:04Z

include/bmi/AbstractCLibBmiAdapter.hpp

-            inline void *dynamic_load_symbol(const std::string &symbol_name) {
-                return dynamic_load_symbol(symbol_name, false);
-            }
+            void *dynamic_load_symbol(const std::string &symbol_name);

-            inline const std::string &get_bmi_registration_function() {
-                return bmi_registration_function;
-            }
+            const std::string &get_bmi_registration_function();

-            inline const void *get_dyn_lib_handle() {
-                return dyn_lib_handle;
-            }


I could've replaced inline with __attribute__((always_inline)) here instead to get the intended behavior, but opted to move definitions to a source file instead because it probably isn't worth it currently.

I would consider keeping at least these three small, simple functions in the header and marked inline. They are relatively simple wrapper functions which are used for each adapter and at scale (800k calls at initialization of conus) could save some overhead in intialization. The library load and load symbol functions are more complex, and inlining them would likely have some similar initializaiton optimizations avoiding additional function calls in setting up the adapters. I don't think these are called from many different places, so the resulting binary size and build times should be minimal.
We may want to at least profile some large init times and see what the overhead in intialization is from NOT inlining these.

Reverted in 7fc98b0

include/bmi/AbstractCLibBmiAdapter.hpp

PhilMiller · 2024-03-14T22:35:11Z

I'm trying to take a look at the ASan failure in the unit test. Needing to update some software locally to do that.

PhilMiller · 2024-03-15T01:00:34Z

Ok, finally re-ran the tests locally with ASan, and didn't see the failure. Going to need some more investigation, I guess.

I may try backing out the very last commit, and see if that changes things

program-- · 2024-03-15T01:34:21Z

Ok, finally re-ran the tests locally with ASan, and didn't see the failure. Going to need some more investigation, I guess.

I may try backing out the very last commit, and see if that changes things

I'm having the same experience. I don't get any ASan errors locally. As a note, it seems master is also experiencing the ASan issues (and PR #720).

PhilMiller · 2024-03-15T02:22:46Z

The ASan failures may be a transient thing due to weird configuration on Github's CI runners. In particular, ASan functionally requires that ASLR not be disabled, because there's certain ELF loading behavior in the Linux kernel that only happens with ASLR enabled. I pushed on that getting fixed about a decade ago, for other reasons, but never got anywhere.

The upshot is that we may see the test start passing if we re-run it again tomorrow.

… subclasses

program-- added the bug Something isn't working label Feb 29, 2024

program-- self-assigned this Feb 29, 2024

program-- marked this pull request as ready for review February 29, 2024 18:38

program-- requested review from PhilMiller, hellkite500 and robertbartel February 29, 2024 18:38

program-- commented Feb 29, 2024

View reviewed changes

PhilMiller reviewed Mar 14, 2024

View reviewed changes

include/bmi/AbstractCLibBmiAdapter.hpp Outdated Show resolved Hide resolved

PhilMiller reviewed Mar 14, 2024

View reviewed changes

include/bmi/AbstractCLibBmiAdapter.hpp Outdated Show resolved Hide resolved

PhilMiller force-pushed the jsm-bmi-refactor branch from 4f83059 to fc557bd Compare March 14, 2024 21:54

program-- mentioned this pull request Mar 16, 2024

Add MPI_Finalize "detouring" mechanism #765

Draft

12 tasks

program-- force-pushed the jsm-bmi-refactor branch from 5e1cb6a to 1a6164e Compare March 22, 2024 16:41

PhilMiller previously approved these changes Mar 22, 2024

View reviewed changes

PhilMiller assigned hellkite500 and unassigned program-- Mar 22, 2024

program-- added 11 commits March 25, 2024 08:11

cmake: refactor bmi listfile to remove dynamic library call

0e0b9c4

bmi: move virtual destructor to Bmi_Adapter, use explicit override on…

2a1b482

… subclasses

bmi: clean up include directives

2b8a554

bmi: more headers; cmake: include bmi subdir in search path

2fada2f

bmi: separate header/impl; fix more includes

fc56fa9

bmi: add missing exception-specifier to AbstractCLibBmiAdapter

48bd831

bmi: actually fix noexcept specifier

afa517d

fix: add FileChecker include to Bmi_Py_Formulation_Test

bd15bde

fix: add FileChecker headerr to Bmi_Py_Adapter_Test

83ded7c

rev: revert move of inline functions to source file

6bfc607

rev: explicitly show that Bmi_Adapter destructor should be overriden

cef3d9c

program-- and others added 4 commits March 25, 2024 08:11

rev: revert constness of AbstractCLiBmiAdapter member variable

28287cf

bmi: Make Bmi_Adapter uniformly non-moveable

4b65be5

bmi: Make all adapter classes 'final'

16999fa

bmi: Move inclusion of dlopen/dlsym header down to .cpp file

c5c6213

PhilMiller dismissed their stale review via c5c6213 March 25, 2024 15:17

PhilMiller force-pushed the jsm-bmi-refactor branch from e96afe2 to c5c6213 Compare March 25, 2024 15:17

Remove extraneous semicolon

efcfa53

hellkite500 approved these changes Mar 25, 2024

View reviewed changes

hellkite500 merged commit 9758d43 into NOAA-OWP:master Mar 25, 2024
19 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: BMI adapters' virtual destructor hierarchy and general cleanup #750

Refactor: BMI adapters' virtual destructor hierarchy and general cleanup #750

program-- commented Feb 29, 2024 •

edited

Loading

program-- left a comment

program-- Feb 29, 2024

hellkite500 Feb 29, 2024

program-- Mar 8, 2024

program-- Feb 29, 2024

program-- Feb 29, 2024

hellkite500 Mar 1, 2024

program-- Mar 8, 2024

PhilMiller commented Mar 14, 2024

PhilMiller commented Mar 15, 2024

program-- commented Mar 15, 2024 •

edited

Loading

PhilMiller commented Mar 15, 2024

		include(${PROJECT_SOURCE_DIR}/cmake/dynamic_sourced_library.cmake)
		dynamic_sourced_cxx_library(ngen_bmi "${CMAKE_CURRENT_SOURCE_DIR}")

Refactor: BMI adapters' virtual destructor hierarchy and general cleanup #750

Refactor: BMI adapters' virtual destructor hierarchy and general cleanup #750

Conversation

program-- commented Feb 29, 2024 • edited Loading

Additions

Changes

Checklist

Target Environment support

program-- left a comment

Choose a reason for hiding this comment

program-- Feb 29, 2024

Choose a reason for hiding this comment

hellkite500 Feb 29, 2024

Choose a reason for hiding this comment

program-- Mar 8, 2024

Choose a reason for hiding this comment

program-- Feb 29, 2024

Choose a reason for hiding this comment

program-- Feb 29, 2024

Choose a reason for hiding this comment

hellkite500 Mar 1, 2024

Choose a reason for hiding this comment

program-- Mar 8, 2024

Choose a reason for hiding this comment

PhilMiller commented Mar 14, 2024

PhilMiller commented Mar 15, 2024

program-- commented Mar 15, 2024 • edited Loading

PhilMiller commented Mar 15, 2024

program-- commented Feb 29, 2024 •

edited

Loading

program-- commented Mar 15, 2024 •

edited

Loading