Breaking changes with PCU #433

flagdanger · 2024-06-05T15:19:39Z

Breaking changes with PCU:

Ported PCU interface to not use global state
- i.e., mesh creation requires passing in a PCU instance
The new interface uses C++
A C interface for PCU still exists in pcu_c.cc
- Create an instance of the C handle struct "PCU_t"
- The original pcu.c functions can be used by passing this struct
Renamed source and headers files:
- PCU.h became PCU_C.h: PCU.h was repurposed as the C++ header
- pcu.c became pcu_c.cc: pcu.c was deleted and pcu_c.cc created without history
- more details on the history is here: Breaking changes with PCU #433 (comment)
Build documentation (make doc) to generate doxygen based docs or see https://www.scorec.rpi.edu/pumi/doxygen/pcu.html for basic PCU.cc usage

This commit adds a MPI state that's separate from the global variables to the internal pcu_mpi interfaces. The user level PCU interface remains unchanged. However, this change lays the groundwork for PCU2 that will not make use of global state. This is important for some use cases like working around the spectrum-mpi comm_dup bug. The removal of global state also improves testability and modularity of this code.

Throughout the code base it was assumed that the mpi functions were implemented through pmpi and so no other implementation really could have been implemented. This change just removes the complexity from having the vtable.

This commit allows the pcu_mpi to be initialized into a stack variable rather than allways allocating on the heap.

This commit adds a MPI state that's separate from the global variables to the internal pcu_mpi interfaces. The user level PCU interface remains unchanged. However, this change lays the groundwork for PCU2 that will not make use of global state. This is important for some use cases like working around the spectrum-mpi comm_dup bug. The removal of global state also improves testability and modularity of this code.

Throughout the code base it was assumed that the mpi functions were implemented through pmpi and so no other implementation really could have been implemented. This change just removes the complexity from having the vtable.

This commit allows the pcu_mpi to be initialized into a stack variable rather than allways allocating on the heap.

flagdanger · 2024-11-20T00:28:37Z

@cwsmith Can you run tests again? Also, the file level doxygen comment that used to be in pcu.c doesn't exist in the new pcu_c.cc file. How can I put it back into the file and make sure it shows up on the scorec doxygen page?

There are still unresolved conflicts with the pull request, but I believe the differences are necessary.

cwsmith · 2024-11-20T01:41:11Z

/runtests

cwsmith · 2024-11-20T01:48:24Z

Also, the file level doxygen comment that used to be in pcu.c doesn't exist in the new pcu_c.cc file. How can I put it back into the file and make sure it shows up on the scorec doxygen page?

The following should work:

git checkout master
cp pcu/pcu.c $HOME/.
git checkout pcu-object-breaking
#copy and paste needed block of text into pcu_c.cc 
git add pcu_c.cc
git commit pcu_c.cc

You can then run 'make doc' (assuming the doxygen binary was in a place cmake could find it) and in the doc directory (in the build directory) there will be html files you point your web-browser at and view.

github-actions · 2024-11-20T01:50:30Z

Build Log
Simmetrix Test Result: success
Simmetrix + CGNS Test Result: success

jacobmerson · 2024-11-21T18:23:38Z

@flagdanger where are we at with the documentation we discussed here: #433 (comment)

Also, looks like there are still some conflicts. Please make sure that the merge has been completed and let us know if you need help finalizing the merge.

flagdanger · 2024-11-21T18:32:51Z

@jacobmerson I can make a draft of documentation at 6 today. Might need review or changes. These conflicts are sticking around after the first merge. I can try again.

jacobmerson · 2024-11-22T00:04:28Z

Cameron and I can work on helping with the merge if needed. Make sure to sync your develop branch with SCOREC/develop before the merge

- My rationale is that these are old ad-hoc tests. - Removed B737 test as well. The CRE is not available so it should not be a test. Signed-off-by: Aiden Woodruff <[email protected]>

fixes SCOREC#465 running valgrind memcheck on a 28M upright mesh generation on 16 ranks reports no leaks on all ranks ==1567914== HEAP SUMMARY: ==1567914== in use at exit: 40 bytes in 1 blocks ==1567914== total heap usage: 293,194,361 allocs, 293,194,360 frees, 27,011,094,588 bytes allocated ==1567914== ==1567914== LEAK SUMMARY: ==1567914== definitely lost: 0 bytes in 0 blocks ==1567914== indirectly lost: 0 bytes in 0 blocks ==1567914== possibly lost: 0 bytes in 0 blocks ==1567914== still reachable: 40 bytes in 1 blocks ==1567914== suppressed: 0 bytes in 0 blocks ==1567914== Rerun with --leak-check=full to see details of leaked memory

Suppress warning by casting to void. Signed-off-by: Aiden Woodruff <[email protected]>

- Moves all of the files that globus uses unto Github. - Moves globus code to separate repository so that it can be reused.

flagdanger · 2024-11-22T01:35:45Z

@jacobmerson I tried merging again, it did not make the conflicts go away.

jacobmerson · 2024-11-26T06:54:01Z

@flagdanger Cameron and I will work on the merge. Are you set with the documentation changes?

jacobmerson · 2024-11-28T04:29:35Z

@cwsmith I think the docs are mostly in order. Take a look and see if there are additional aspects you want. Let's touch base after Thanksgiving to do the final merge.

cwsmith · 2024-12-02T15:54:07Z

The documentation looks good. I reworked the opening description #433 (comment) to be more verbose. I'll resolve the merge conflicts and check the delta wing results.

jacobmerson · 2024-12-02T18:07:19Z

Thanks!

cwsmith · 2024-12-03T21:26:36Z

@jacobmerson ~~Please take a look at this PR when you get a chance. It should fix the remaining merge conflicts. jacobmerson#4~~ Nevermind, using the web interface was cleaner.

cwsmith · 2024-12-04T00:28:26Z

/runtests

github-actions · 2024-12-04T00:37:13Z

Build Log
Simmetrix Test Result: success
Simmetrix + CGNS Test Result: success

cwsmith · 2024-12-04T03:05:45Z

The deltawing test run on piglet (32 procs) was 2.7% slower with this branch (450s) than develop (438s). Each test was run twice and had consistent performance. This seems to be more than 'noise', but I think we should proceed with the merge. Thoughts @jacobmerson ?

The respective source for the tests are here:
https://github.com/SCOREC/core/tree/cws/developDeltaAdapt @ 8dc400d
https://github.com/SCOREC/core/tree/cws/pcu-object-breaking_deltaAdapt @ bedb3c2

cmake - same flags and options for both builds

cmake -S /space/cwsmith/pumi30Release/core -B buildPumiOptonSimonOmegaoff_develop -DBUILD_SHARED_LIBS=off -DCMAKE_C_COMPILER=mpicc -DCMAKE_CXX_COMPILER=mpicxx -DCMAKE_Fortran_COMPILER=gfortran -DSCOREC_CXX_OPTIMIZE=on -DSCOREC_EXTRA_CXX_FLAGS= -DVALGRIND= -DVALGRIND_ARGS= -DMDS_ID_TYPE=int -DPCU_COMPRESS=ON -DENABLE_ZOLTAN=ON -DPUMI_FORTRAN_INTERFACE=off -DENABLE_OMEGA_H=off -DIS_TESTING=ON -DENABLE_SIMMETRIX=on -DSKIP_SIMMETRIX_VERSION_CHECK=on -DENABLE_FIELDSIM=OFF -DS
IM_MPI=mpich4.1.1 -DSIM_PARASOLID=on -DSIM_ACIS=on -DMESHES=/space/cwsmith/pumi30Release/core/pumi-meshes/ -DCMAKE_INSTALL_PREFIX=buildPumiOptonSimonOmegaoff_develop/install

partition

mpirun -np 32 ./zsplit /lore/smithc11/projects/deltaWingAdapt/pumiAdapt/deltaWing.dmg /lore/smithc11/projects/deltaWingAdapt/500kMetric/deltaWing_500kMetric.smb p32/ 32

run

#!/bin/bash -ex
p=$1
bin=/space/smithc11/deltaWing/anisoDelta_ma_test
mesh=p${p}/
mdl=/lore/smithc11/projects/deltaWingAdapt/pumiAdapt/deltaWing.dmg
mpirun -np $p $bin $mdl $mesh

jacobmerson · 2024-12-04T04:20:53Z

I think we should proceed even if we cannot track that down as I think getting rid of the singleton is quite important. Nothing obvious jumps out to me as what should cause a slow down. Maybe just chasing a pointer instead of having something in global memory? 2% may be hard to see in a debug profile, but it might be worth a try.

jacobmerson · 2024-12-04T14:00:17Z

@cwsmith should -DCMAKE_BUILD_TYPE be set to release in the build script?

cwsmith · 2024-12-04T16:29:40Z

@jacobmerson By default the pumi build system uses -O2 -g (via these options). The compile and link line for the delta wing driver are below. Do you think -O3 without debug symbols will make a difference?

cd /space/cwsmith/pumi30Release/buildPumiOptonSimonOmegaoff_30/test && /opt/scorec/spack/rhel9/v0201_4/install/linux-rhel9-x86_64/gcc-12.3.0/mpich-4.1.1-xpoyz4tqgfxtrm6m7qq67q4ccp5pnlre/bin/mpicxx -DHAVE_CLOCK_GETTIME -DHAVE_SIMMETRIX -DOMPI_SKIP_MPICXX -DPUMI_HAS_ZOLTAN <...all the include paths> -O2 -g  -std=c++11 -MD -MT test/CMakeFiles/anisoDelta_ma_test.dir/anisoDelta_ma_test.cc.o -MF CMakeFiles/anisoDelta_ma_test.dir/anisoDelta_ma_test.cc.o.d -o C     MakeFiles/anisoDelta_ma_test.dir/anisoDelta_ma_test.cc.o -c /space/cwsmith/pumi30Release/core/test/anisoDelta_ma_test.cc
[100%] Linking CXX executable anisoDelta_ma_test
cd /space/cwsmith/pumi30Release/buildPumiOptonSimonOmegaoff_30/test && /opt/scorec/spack/rhel9/v0201_4/install/linux-rhel9-x86_64/gcc-12.3.0/cmake-3.26.3-2duxfcdiykykfynkadpp2m2y6zrmuvz4/bin/cmake -E cmake_link_script CMakeFiles/anisoDelta_ma_test.dir/link.txt --verbose=1
/opt/scorec/spack/rhel9/v0201_4/install/linux-rhel9-x86_64/gcc-12.3.0/mpich-4.1.1-xpoyz4tqgfxtrm6m7qq67q4ccp5pnlre/bin/mpicxx  -O2 -g  CMakeFiles/anisoDelta_ma_test.dir/anisoDelta_ma_test.cc.o -o anisoDelta_ma_test   <... all the libs>

jacobmerson and others added 30 commits March 28, 2023 04:14

Compile on gcc+fix bug

7c390ee

remove virtual function table for pcu_mpi

a4bbbfb

Throughout the code base it was assumed that the mpi functions were implemented through pmpi and so no other implementation really could have been implemented. This change just removes the complexity from having the vtable.

Put PCU state data into a structure

baf9370

Use consistent initialization functions for MPI.

4c243cb

This commit allows the pcu_mpi to be initialized into a stack variable rather than allways allocating on the heap.

Add PCU2 interface that takes the PCU state struct

d891d2b

add PCU2.h to install files

de2f57d

fix compiler warnings

6a46101

constify mpi functions

ed28c05

first pass of PCU with making use of a high level object

2eada4e

remove unused functions

eed5a0c

Fix gcc compilation

f0b3a28

fix bug

0e7362f

remove unnecessary whitespace

0e0c80e

bump version

07ab364

Compile on gcc+fix bug

2f6238c

remove virtual function table for pcu_mpi

a5be7ae

Throughout the code base it was assumed that the mpi functions were implemented through pmpi and so no other implementation really could have been implemented. This change just removes the complexity from having the vtable.

Put PCU state data into a structure

064ea13

Use consistent initialization functions for MPI.

e727d54

This commit allows the pcu_mpi to be initialized into a stack variable rather than allways allocating on the heap.

Add PCU2 interface that takes the PCU state struct

47bf6d5

add PCU2.h to install files

ef021f6

fix compiler warnings

270d44e

constify mpi functions

62bfb69

first pass of PCU with making use of a high level object

173c7ec

remove unused functions

23c7cef

Fix gcc compilation

438a73e

fix bug

4fa6e37

remove unnecessary whitespace

716133e

bump version

4d281e0

flagdanger and others added 5 commits November 21, 2024 19:43

PCU.cc documentation

5512298

Remove exe sources which are not in CMake

3f6b3d4

- My rationale is that these are old ad-hoc tests. - Removed B737 test as well. The CRE is not available so it should not be a test. Signed-off-by: Aiden Woodruff <[email protected]>

Fix unused variable warning introduced by SCOREC#446 (SCOREC#453)

42dd756

Suppress warning by casting to void. Signed-off-by: Aiden Woodruff <[email protected]>

test globus (SCOREC#466) - refactor globus compute scripts

0d3aaf9

- Moves all of the files that globus uses unto Github. - Moves globus code to separate repository so that it can be reused.

jacobmerson mentioned this pull request Nov 28, 2024

Changes for sdk approval SCOREC/pumi-pic#132

Merged

Merge branch 'develop' into pcu-object-breaking

7e54d7f

cwsmith closed this Dec 4, 2024

cwsmith reopened this Dec 4, 2024

cwsmith merged commit bd3721b into SCOREC:develop Dec 6, 2024
4 checks passed

cwsmith mentioned this pull request Dec 6, 2024

PUMI 3.0.0 Release #467

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Breaking changes with PCU #433

Breaking changes with PCU #433

flagdanger commented Jun 5, 2024 •

edited by cwsmith

Loading

flagdanger commented Nov 20, 2024

cwsmith commented Nov 20, 2024

cwsmith commented Nov 20, 2024 •

edited

Loading

github-actions bot commented Nov 20, 2024

jacobmerson commented Nov 21, 2024 •

edited

Loading

flagdanger commented Nov 21, 2024

jacobmerson commented Nov 22, 2024

flagdanger commented Nov 22, 2024

jacobmerson commented Nov 26, 2024

jacobmerson commented Nov 28, 2024

cwsmith commented Dec 2, 2024

jacobmerson commented Dec 2, 2024

cwsmith commented Dec 3, 2024 •

edited

Loading

cwsmith commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

cwsmith commented Dec 4, 2024 •

edited

Loading

jacobmerson commented Dec 4, 2024 •

edited

Loading

jacobmerson commented Dec 4, 2024

cwsmith commented Dec 4, 2024

Breaking changes with PCU #433

Breaking changes with PCU #433

Conversation

flagdanger commented Jun 5, 2024 • edited by cwsmith Loading

flagdanger commented Nov 20, 2024

cwsmith commented Nov 20, 2024

cwsmith commented Nov 20, 2024 • edited Loading

github-actions bot commented Nov 20, 2024

jacobmerson commented Nov 21, 2024 • edited Loading

flagdanger commented Nov 21, 2024

jacobmerson commented Nov 22, 2024

flagdanger commented Nov 22, 2024

jacobmerson commented Nov 26, 2024

jacobmerson commented Nov 28, 2024

cwsmith commented Dec 2, 2024

jacobmerson commented Dec 2, 2024

cwsmith commented Dec 3, 2024 • edited Loading

cwsmith commented Dec 4, 2024

github-actions bot commented Dec 4, 2024

cwsmith commented Dec 4, 2024 • edited Loading

cmake - same flags and options for both builds

partition

run

jacobmerson commented Dec 4, 2024 • edited Loading

jacobmerson commented Dec 4, 2024

cwsmith commented Dec 4, 2024

flagdanger commented Jun 5, 2024 •

edited by cwsmith

Loading

cwsmith commented Nov 20, 2024 •

edited

Loading

jacobmerson commented Nov 21, 2024 •

edited

Loading

cwsmith commented Dec 3, 2024 •

edited

Loading

cwsmith commented Dec 4, 2024 •

edited

Loading

jacobmerson commented Dec 4, 2024 •

edited

Loading