libCEED icon indicating copy to clipboard operation
libCEED copied to clipboard

fluids: Unify Newtonian Primtive functions

Open jrwrigh opened this issue 3 years ago • 2 comments

  • Combines the separate primitive QFunctions in newtonian.h into a single function call.
  • Replaces StateFromQi* function pointers with QF helper functions using if statements inside.
  • Includes various other refactors and stylings.

Stripped this out from #1031 while that is a bit stalled for the moment. Figured it might as well be it's own PR

jrwrigh avatar Jul 31 '22 22:07 jrwrigh

Thanks, this looks good from a coding standpoint. Can you do a basic test to identify whether there is any performance implications to this on CPU or GPU?

jedbrown avatar Aug 01 '22 04:08 jedbrown

We're actually seeing a more significant performance difference that I was expecting. Running

./build/fluids-navierstokes -ceed /gpu/hip -options_file examples/fluids/channel.yaml -dm_plex_box_faces 20,20,1 -ts_max_steps 20 -output_freq 0

I'm seeing 3% slower for CPU (19.15 -> 19.75 seconds) and 0.5% for HIP on Noether (59.6 -> 59.9). This is trend is consistent after running a few more times.

I figured the branch prediction on the CPU would prevent it from being a significant factor there and any performance difference would be more evident on the GPU (though it very well could just be that at this problem size the consequential speed decrease is masked by other slow-downs).

Any idea why this is consequential by the case statements for the stabilization are (presumably) not? Or is there a way to tell the compiler that branch will be identical through a run (though I'd be surprised if there was a way to do that portably)?

jrwrigh avatar Aug 01 '22 17:08 jrwrigh

A couple of code opinion/style questions:

  1. I've kept the if statement versions of StateFromQi. I think we had agreed that having these as slightly non-optimal for the boundary QFs is ok since they're not the dominant cost. I've still kept them here and only moved the volume QFs to the pointer method. We still good with that?

  2. For the function pointers passed into [IFunction,IJacobian]_Newtonian_Generic, I've currently named as pStateFromQi(_fwd) emphasize that they're different than the StateFromQi* helper functions (p meaning "pointer"). I originally had the function pointers named as StateFromQi* since they have the exact same interface and (intended) behavior as the helper functions. I'm open to switching back or giving them a different name. Thoughts?

jrwrigh avatar Aug 24 '22 21:08 jrwrigh