search

idaholab / moose

Multiphysics Object Oriented Simulation Environment
https://www.mooseframework.org
GNU Lesser General Public License v2.1
1.79k stars 1.05k forks source link

Make unity build able to create unity of a folder that contains sub-folders #26028

Closed YaqiWang closed 10 months ago

YaqiWang commented 1 year ago

Reason

Currently with this https://github.com/idaholab/moose/blob/7b18119f1d64c2de6a128981de4af4894299f47a/framework/moose.mk#L296, unity build will create unity files for all folders. Sometimes, it is desired to create a single unity file for a folder even the folder contains subfolders. This can help accelerating compiling even further if the folder structure is deep. We have an example in Griffin:

Compiling C++ (in dbg mode) /Users/xxx/projects/griffin/radiation_transport/build/unity_src/transportsystems_DIFFUSION-DFEM_Unity.C...
Compiling C++ (in dbg mode) /Users/xxx/projects/griffin/radiation_transport/build/unity_src/transportsystems_DIFFUSION-DFEM_kernels_Unity.C...
Compiling C++ (in dbg mode) /Users/xxx/projects/griffin/radiation_transport/build/unity_src/transportsystems_DIFFUSION-DFEM_bcs_Unity.C...
Compiling C++ (in dbg mode) /Users/xxx/projects/griffin/radiation_transport/build/unity_src/transportsystems_DIFFUSION-DFEM_interfacekernels_Unity.C...

where the current make system will create unity files for all folders including non-leaf folders like griffin/radiation_transport/src/transportsystems/DIFFUSION-DFEM. The corresponding unity file griffin/radiation_transport/build/unity_src/transportsystems_DIFFUSION-DFEM_Unity.C is empty. It is desired to not create the unity files for its subfolders but merge those three unity files into griffin/radiation_transport/build/unity_src/transportsystems_DIFFUSION-DFEM_Unity.C.

Design

If application developers can specify the depth of the unity build for a particular folder, it will resolve this issue. Tag @permcody

Impact

Make unity build even better for more complicated source folder structures.

permcody commented 1 year ago

@YaqiWang - I have a pretty simple patch which turns on more unity for everyone. The simple patch just builds unity files for all top level src directories instead of all folders in src like we do now. This is probably closer to what we want for everyone. Ironically, ISOXML is one of the few (only) applications that's breaking the current patch. The issue is that we do have flexibility to turn off unity on specific directories. I don't think we do that too often, but we do have src and src/base in that list right now. With the simple patch I've created, we aren't picking up source files in directories underneath folders we've turned off...

Clearly this is enough of an issue that at the very least I'd need to detect and warn or even error when this condition is detected, but I'm not so sure I want to write a ton of extra logic to support it. Think I'd rather just error. Will you take a look at the ISOXML structure and see if the current structure makes sense? Does having that subfolder under base make sense? I'll push up this patch later today and see how it does against all apps.

UPDATE: I think we'll likely just take base out of the global "off" list. That would just fix this issue.

YaqiWang commented 1 year ago

Do you mean PebbleBed folder under isoxml/src/base? No, it is created accidentally. It skipped our review process, otherwise the two files under it will be just in src/base.

permcody commented 1 year ago

Yes, PebbleBed. I don't think you'll have to remove it for this patch to work. I'm going to remove base as a skip folder so it won't matter. I did find one other tiny patch for IsoXML that will need to be merged though. Doing timings now and will update here.

permcody commented 1 year ago

Current Griffin Build: make -j10 1286.05s user 196.03s system 569% cpu 4:20.24 total

More Unity: make -j10 691.59s user 117.54s system 464% cpu 2:54.05 total