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

Remove global variables #2628

Open
jngrad opened this issue Mar 24, 2019 · 4 comments
Open

Remove global variables #2628

jngrad opened this issue Mar 24, 2019 · 4 comments

Comments

@jngrad
Copy link
Member

jngrad commented Mar 24, 2019

Global variables make code difficult to read and maintain. Tracking their origin is an annoyance, and name collision with local variables is always a risk. Should we consider prefixing global variables with g_, just like we prefix class member variables with m_ and enum values with their enum name? Or enclosing them in namespaces, like enum values from enum classes?

Here is one special example, hiding in the global scope of some LB code:

Documentation is also affected by this situation, because Doxygen doesn't have a reliable scope resolution mechanism (Doxygen incorrectly states this variable is referenced by 380 functions).

@fweik
Copy link
Contributor

fweik commented Mar 25, 2019

The one you mention is also a linking hazard. In general most should also be static.

@jngrad
Copy link
Member Author

jngrad commented Jul 16, 2019

Other example: there is a global variable energy:

extern Observable_stat energy, total_energy;

which can easily be shadowed in functions calculating energies, forcing the use of alternative variable names, like _energy, eng, ret.

Removing globals is also an option: e416e7e, d47a731, 34b8821, fab58ca, 6b56534

bors bot added a commit that referenced this issue Sep 12, 2019
3155: Fix regressions found by exotic archs r=KaiSzuttor a=jngrad

Part of the [release checklist](https://github.com/espressomd/espresso/wiki/release-checklist#for-all-releases): run tests on exotic architectures and intel compiler.

3161: Core: do_non_bonded, capture by reference rather than value r=jngrad a=RudolfWeeber

This fixes a performance regression from #3077, I think.
Time per step fro lj benchmark went down from 0.78 to 0.38 ms

3162: Cleanup LB global variables r=KaiSzuttor a=jngrad

Follow-up to #2628

Description of changes:
- removed unused global variables in LB code
- cleaned up documentation

Co-authored-by: Jean-Noël Grad <[email protected]>
Co-authored-by: Rudolf Weeber <[email protected]>
@jngrad jngrad changed the title Add prefix to global variables Add prefix to global variables, or gather them in structs, or pass them as function arguments Jan 25, 2020
kodiakhq bot added a commit that referenced this issue Jan 27, 2020
- Gather thermostat global variables in structs (#2628)
- Make thermostat functions pure functions to eliminate side effects
- Add Boost unit tests for thermostats
- Reduce thermostat code complexity
@jngrad
Copy link
Member Author

jngrad commented Feb 19, 2020

Progress report:

kodiakhq bot added a commit that referenced this issue May 27, 2020
kodiakhq bot added a commit that referenced this issue Jun 24, 2020
Description of changes:
- re-use code in the cython interface to extract data from `Observable_stat` objects
- remove `obs_scalar_pressure` global variable
   - write-only variable in the core -> can be calculated from `obs_pressure_tensor` in cython
   - simplify pressure kernels
   - less MPI communication
- rename `obs_pressure_tensor` to `obs_pressure` for consistency with `obs_energy`
- remove external linkage for  `obs_pressure` and `obs_energy`
   - these globals are no longer visible outside of their translation unit
   - use a const reference getter for the script interface
   - partial fix for #2628
- narrow includes of `Observable_stat.hpp`
- add ELC/DLC energy corrections to the P3M/DP3M/DDS solver energy slots
   - the `analyze.energy()`, `analyze.pressure()`, `analyze.pressure_tensor()` functions now only reports 2 slots for Coulomb and dipolar contributions: particles contribution and electrostatics/magnetostatics solver contribution
kodiakhq bot added a commit that referenced this issue Jan 29, 2021
Description of changes:
- convert CUDA error codes into runtime errors using the message associated to the error code
- properly handle CUDA errors by halting the flow of the program instead of ignoring them (Barnes-Hut, LB GPU)
- remove superfluous CUDA global variables (partial fix for #2628)
- restore `cuda_gather_gpus()` functionality and convert the `device_list()` getter to a regular function (API change)
- remove unused `libcuda` dependency (fixes #4085)
@jngrad jngrad changed the title Add prefix to global variables, or gather them in structs, or pass them as function arguments Remove global variables, or gather them in structs, or pass them as function arguments Mar 12, 2021
@jngrad
Copy link
Member Author

jngrad commented Apr 16, 2021

Progress report:

kodiakhq bot added a commit that referenced this issue Jun 8, 2021
Partial fix for #2628

Description of changes:
- pass `time_step` and `sim_time` as function arguments
- make `skin_set` and `verlet_reuse` static
- move logic from globals.pyx to the relevant .pyx files
- API changes:
   - member `system.globals` no longer exists
   - several electrostatic/magnetostatic methods now have a `timings` optional argument to control the number of force calculations during tuning
kodiakhq bot added a commit that referenced this issue Aug 12, 2021
Partial fix for #2628

Description of changes:
- make the `Observable_stat` class self-contained
- remove the `Observable_stat` global variables
- reduce code complexity
kodiakhq bot added a commit that referenced this issue Dec 23, 2021
Partial fix for #2628 and #4219

Description of changes:
- simplify CUDA kernels and GPU interface code
   - remove superfluous GPU-related macros and unused code
   - around 300 lines of CUDA code were deleted
- remove GPU magnetostatics global variables
   - they are now shared pointers in the Cython interface
- rewrite `SystemInterface` framework
   - document framework
   - remove unused methods
   - remove broken past-the-end device memory pointers (access to device memory with the wrong alignment is dangerous)
   - throw an error when requesting a particle list conversion from AoS to SoA for features that are not compiled in
   - unit test `EspressoSystemInterface` implementation
- improve separation of concerns:
   - `magnetostatics.pyx` no longer knows about the `dipole` global nor the associated enum values
   - header files of GPU long-range methods no longer expose implementation details
   - narrow includes of `cells.hpp`, `thermostat.hpp`, `rotation.hpp` to the strict minimum
      - the majority of indirect includes were unnecessary, exposed global variables and increased compile time
      - where necessary, functions were moved to different files and global variables `this_node` and `cell_structure` were forwarded by function argument
- bugfixes:
   - fix device memory leak in GPU Barnes-Hut (can fill up device memory quickly when creating, using and then deleting a Barnes-Hut actor in a loop)
   - GPU dipolar direct sum and Barnes-Hut used to emit a runtime error message, but `handle_errors()` was missing in the Cython interface
kodiakhq bot added a commit that referenced this issue May 31, 2022
Fixes #4454

Description of changes:
- apply the SOLID principles
   - split electrostatics from magnetostatics
   - factor out code duplication
   - extract P3M/DP3M tuning code
   - hide ELC details in private class methods (the energy/force correction logic used to be exposed in `coulomb.cpp`)
   - hide Coulomb implementation details by no longer indirectly including `coulomb.hpp` in 30 unrelated files in the core
- write clear pre-conditions as event hooks
   - these events are triggered upon box size change, periodicity change, node grid change and cell structure change
      - partial fix for #4428
      - setting a `node_grid` incompatible with a long-range actor is no longer possible (the old `node_grid` is restored)
   - these preconditions always run once before the integration loop starts
   - write test cases to check for these events
   - bugfix: charge neutrality checks now run once before the integration loop starts, instead of once at actor activation
- encapsulate the state of long-range methods in classes managed by the script interface
   - bugfix: the cached `DipolarP3M` energy correction is now invalidated when the box length changes
   - remove global variables for CPU methods (partial fix for #2628) and introduce one global variable for each type of long-range actor: dipolar solver, Coulomb solver, Coulomb extension
  - convert Cython files for long-range actors to Python files using the `ScriptInterface` framework
     - long-range actors and their python object now have the same lifetime, regardless of whether the actor is active or not (partial fix for #4220)
  - the list of active long-range actors can no longer end up in an invalid state (partial fix for #4219):
     - updating an active actor with an invalid parameter now automatically restores the original parameter
     - inserting an invalid long-range actor in the list of actors now automatically removes the actor from the list of active actors, even when the exception happens during tuning
- API changes:
   - `ELC` and `DLC` are no longer extensions, instead they take another actor as argument
   - `P3MGPU` is now able to execute the CPU energy and pressure kernels using parameters tuned for the GPU force kernel
      - closes #239
      - P3M-based reaction scripts run with 2x speed-up when reaction steps take place every 1000 integration steps
   - `P3MGPU` is now supported by `ELC`
   - long-range actor parameters are now immutable, since changing one parameter (e.g. the prefactor) would usually invalidate the tuned parameters
   - the charge neutrality check sensitivity can now be modified via `actor.charge_neutrality_tolerance`; this is mostly relevant to actors coupled to `ICC`, whose induced charges can have value spanning several orders of magnitude
- under-the-hood changes:
   - CPU-based long-range actors no longer rely on the `MpiCallbacks` framework
   - the subtracted Coulomb bonded interaction and pair criteria no longer indirectly include `coulomb.hpp`
kodiakhq bot added a commit that referenced this issue Apr 24, 2023
Fixes #4712

Description of changes:
- rewrite integrator as Python (partial fix for #4542)
- remove two MPI callbacks (partial fix for #4614)
- remove a global variable (partial fix for #2628)
@jngrad jngrad changed the title Remove global variables, or gather them in structs, or pass them as function arguments Remove global variables Jun 12, 2023
kodiakhq bot added a commit that referenced this issue Jun 16, 2023
Fixes #4615

Description of changes:
- completely rewrite the `EspressoSystemInterface` class
   - the new `System` class uses composition for GPU particle data management, resource deallocation, and globals
   - GPU particle data management now uses standard `thrust` vectors and properly deallocates device memory
   - simplify the `espressomd.system.System` class
- simplify GPU code
   - 9 CUDA global variables were removed (partial fix for #2628)
   - ~300 lines of CUDA code were removed
kodiakhq bot added a commit that referenced this issue Jul 13, 2023
Description of changes:
- make electrostatics a member of the System class
- make magnetostatics a member of the System class
- remove 5 global variables (partial fix for #2628)
- API change: new syntax for long-range actors
```python
system.electrostatics.solver = espressomd.electrostatics.P3M(...)
system.electrostatics.extension = espressomd.electrostatic_extensions.ICC(...)
system.magnetostatics.solver = espressomd.magnetostatics.DipolarDirectSumCpu(...)

system.electrostatics.clear()
system.magnetostatics.clear()
```
kodiakhq bot added a commit that referenced this issue Aug 4, 2023
Description of changes:
- remove two P3M global variables (partial fix for #2628)
- add missing `cudaFree`  to fix all remaining memory leaks on the GPU
- add a new `pypresso` option `--cuda-sanitizer` to run the NVIDIA Compute Sanitizer suite
   - by default, runs `memcheck`, but one can select another tool via command line options
   - the stand-alone tool `cuda-memcheck` is deprecated in CUDA 12.0
kodiakhq bot added a commit that referenced this issue Aug 22, 2023
Description of changes:
- API changes:
   - the `espressomd.System` class now has a new member `lb`
   - the `espressomd.System` class member `ekreactions` is now the `reaction` attribute of `system.ekcontainer`
   - the `espressomd.System` class no longer has an `actors` member
   - further API changes will take place in an upcoming PR
- remove Boost dependency from LB/EK sources
   - follow-up to [**Remove remaining boost components** walberla/walberla#605](https://i10git.cs.fau.de/walberla/walberla/-/merge_requests/605)
   - partial fix for #4723 (comment)
   - unit tests from the walberla bridge still depend on `Boost::unit_test_framework`
- fix regressions in EK
   - EK now throws an error when the box_l or node_grid changes, when the MD time step and EK tau disagree, when the box size is not an integer multiple of agrid
- rewrite the LB and EK code in the core from scratch to enforce SOLID principles
   - folder `src/grid_based_algorithms` was split into `src/core/lb` and `src/core/ek`
   - LB and EK global variables (`lattice_switch`, `ek_container`, `ek_reactions`, `lb_walberla_instance`,
   `lb_walberla_params_instance`) were removed; these objects are now members of the `System` class
    (partial fix for #2628)
kodiakhq bot added a commit that referenced this issue Oct 17, 2023
Partial fix for #2628

Description of changes:
- encapsulate cell structure, box geometry and local box in the `System` class
- gather MPI-related global variables in a global struct
- rewrite position folding/unfolding free functions as member functions of `BoxGeometry`

I took this opportunity to cleanup and modernize code, by e.g. replacing hard-coded loops by `Utils::Vector` operations, improving type safety (no mixed signed and unsigned int in arithmetic expression), using more expressive variable names, passing global variables by function argument, adding a collision detection test case, etc.
kodiakhq bot added a commit that referenced this issue Nov 20, 2023
Description of changes:
- encapsulate several global variables inside the `System` class
   - new members: cell_system, integrator, electrostatics, magnetostatics, analysis, comfixed, galilei, bond_breakage
   - required for #4613
   - partial fix for #2628
- make energy/pressure/force calculation functions members of the `System` class
- fully encapsulate event hooks in the `System` class
- simplify checkpointing of the `System` class
- remove `EspressoSystemStandAlone` class
- API changes:
   - `espressomd.galilei.GalileiTransform` is now a member of the `System` class, accessible via `system.galilei`
kodiakhq bot added a commit that referenced this issue Jan 19, 2024
Description of changes:
- encapsulate thermostats and remove their Cython interface
   - use one C++ `ScriptInterface` class per core class (fixes #3980)
   - introduce the `ThermostatFlags` enum, allow deactivating a single thermostat, add an `is_active` flag accessible from the Python interface (fixes #4266)
- remove 20 MPI callbacks
   - partial fix for #4614
- remove thermostat and thermalized/rigid bond global variables
   - partial fix for #2628
- remove a significant amount of historical Cython code that relied on deprecated Cython features
   - partial fix for #4542
   - ESPResSo is now compatible with Cython 3.0
   - the `nogil` deprecation warning subsists
- improve testing of the thermostats interface
- API changes:
   - thermalized bond parameter `seed` was moved to `system.thermostat.set_thermalized_bond()`
      - this change better reflects the fact there is only one global seed for all thermalized bonds
      - until now this global seed was overwritten by any newly created thermalized bond, whether it was added to the system or not
   - LB thermostat `seed` parameter now gets written to the RNG seed (silent change)
      - until now, the RNG seed was hardcoded as `0` and the `seed` value was used to set the RNG counter, thus two independent simulations with a different seed would actually get the same particle coupling random noise sequence with a time lag equal to the difference between the two seeds
      -  this is a bugfix for ensemble runs
      - fixes #4848
kodiakhq bot added a commit that referenced this issue Aug 1, 2024
Fixes #4821
Partial fix for #2628
Partial fix for #4613

Description of changes:
- encapsulate non-bonded interactions, bonded interactions, collision detection, particle list, cluster structure analysis, object-in-fluid, immersed boundaries, auto-update accumulators, constraints, MPI-IO, MMM1D
- API changes:
   - `Mpiio` and `ClusterStructure` now take a system object as argument
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants