EOS copies impacting performance with small array calls

[Yannicked]

I’m seeing a performance issue when calling the EOS routines on many small arrays (≈8 elements each). Profiling shows that every call to

my_eos.InternalEnergyFromDensityTemperature(density, temperature)

invokes the EOS copy constructor. Since the SpinerEOS object is about 20 KB (per sizeof), these repeated copies add significant overhead. It appears that PR #106 introduced an explicit copy in this path.

[Yurlungur]

This copy is necessary on GPUs as references and pointers don't span the two memory spaces. We could potentially remove this copy in the serial case...

I am a little reluctant to add such a branch, as it will make debugging more difficult. But maybe the switch can be based on portability strategy... I.e., for Kokkos, behavior would be unchanged but for PORTABILITY_STRATEGY_NONE we could short circuit this. Would that work?

[Yurlungur]

Alternatively, if you are trying to run on GPUs, you are passing the kernel launch overhead. We should talk about your specific use case, as you may want to restructure your code.

[Yannicked]

I’m running in serial right now, so no GPU (yet). What if we used KOKKOS_CLASS_LAMBDA (i.e. [=, *this], instead of [=]) to capture and copy *this? Would that skip the extra copy in serial (PORTABILITY_STRATEGY_NONE) while still working on device?

[Yurlungur]

What if we used KOKKOS_CLASS_LAMBDA

That would avoid 1 of 2 extraneous copies, as even in serial KOKKOS_CLASS_LAMBDA is [=, *this], meaning the whole class is captured by value. Would that be sufficient to resolve your performance issues? It would also mean that we couldn't use CRTP here (as it requires accessing the this pointer either on host or device to do the typecast) but that's probably fine for these particular loops.

The alternative is something like this:

#ifdef PORTABILITY_STRATEGY_NONE
#define SG_MAYBE_REF(...) __VA_ARGS__&
#else
#define SG_MAYBE_REF(...) __VA_ARGS__
#endif

and then calling

SG_MAYBE_REF(CRTP) copy = ...

[jhp-lanl]

I'm a bit confused here... why does the entire class need to be copy captured? It seems like we need to copy it to device and then copy capture the pointer to the class on the device.

It sounds from @Yannicked 's original post that a class copy is being created for every lambda capture.

[Yurlungur]

I'm a bit confused here... why does the entire class need to be copy captured?

There's a few entangled pieces of this:

1. To access the member functions on device, we at least need to do [this] because otherwise the this pointer isn't in scope, and thus class methods will not be available. That's a C++ism
2. But we can't capture this on device because it points to the wrong memory. Hence why KOKKOS_CLASS_LAMBDA captures *this. But that has the effect of capturing the entire class by value, and doing an extra copy.
3. On top of that, to do CRTP, we need the this pointer so that we can recast the type of the EOS. So we either need the this pointer on device or we need to manipulate it on the host.

We could do

CRTP *pcopy = static_cast<const CRTP*>(this);
portableFor("name", N, [=, *pcopy] { blah });

I think. That would eliminate one extraneous copy but not both (as we'd still be dereferencing pcopy and capturing it on device).

We could also do the ugly trick I listed above, where when it's safe to do so, we capture CRTP copy by reference instead of value.

After experimenting a bit today, I don't think actually that we can avoid doing CRTP with the design we chose here. We need access to the object as the child type.

[Yurlungur]

I am increasingly leaning towards the MAYBE_REF idea. As it avoids all copies when possible and if you have multiple memory spaces you have to copy anyway.

[dholladay00]

With the code you have above @Yurlungur, could we always do CRTP* copy = static cast ...;? If so, that's a change we could make w/o too much difficulty.

portableFor("name", N, [=, *pcopy] PORTABLE_FUNCTION { ... });

I think the above should work globally. Assuming that deference correctly puts things on device.

[Yurlungur]

could we always do CRTP* copy = static cast ...;? If so, that's a change we could make w/o too much difficulty.

portableFor("name", N, [=, *pcopy] PORTABLE_FUNCTION { ... });

I think the above should work globally. Assuming that deference correctly puts things on device.

Ooh clever. Yeah I think that would eliminate 1 of the 2 copies and still allow us to do CRTP. If we wanted to eliminate both copies, though, we'd need to do my reference thing I think.

[Yurlungur]

Ok so it turns out that we can simply assign the CRTP by reference, as [=] in the lambda implies assignment operator and so the underlying object is captured by value even if it is a reference in scope.

@Yannicked see if MR #509 improves performance sufficiently. There is still an implicit copy in the PORTABLE_LAMBDA and this would not be improved by changing to KOKKOS_CLASS_LAMBDA. However, if you're still having issues, there's additional things we could try:

1. We could switch the lambda semantics to [&] on CPU
2. We could implement a custom fortran function for host-side vector calls that does not copy.

[Yurlungur]

It's worth noting this is only an issue in the vector API (which is all we expose to fortran). With the scalar API available in C++ there is no such issue.