Wrap track creation in Actions to get timing

[sethrj]

Wrap track creation in Actions to get timing

[drbenmorgan]

The secondaries looks straightforward in that it really does look like it just needs the function to be wrapped as it only takes a CoreRef argument. Primaries might be trickier as that takes a CoreRef and the span of primaries. I'll start with the secondaries and go from there.

[sethrj]

@drbenmorgan I don't think I thought through my original comment very well: it doesn't makes sense to me now to wrap extend_from_primaries as an action since it doesn't happen at every step. We could make a separate timer for it later but I'm quite satisfied with just having the initialize/extend_from_secondaries as actions. Thanks for that!

[drbenmorgan]

@sethrj, if no one else is assigned to this/alrwady looking, I'd be happy to have a go at this as does look straightforward, at least writing the Actions themselves. I guess the Action(s) should go in celeritas/track?

[sethrj]

Great thanks! I think that's just right. Hopefully will be straightforward?

[sethrj]

Good point. Since we have the secondary initializers in the global state, we might consider just putting the primary initializers in the track state as well?

[drbenmorgan]

Hmm, don't think I understand the data structures/sequence of operations enough to know if that'd be reasonable/work.

In the meantime, the other issue from a naive implementation:

class ExtendFromSecondariesAction final : public ExplicitActionInterface
{
  public:
    //! Execute the action with host data
    void execute(CoreHostRef const& core) const final
    {
        extend_from_secondaries(core);
    }
...

is that the Action execute member functions take a CoreRef<M> const& whilst extend_from_secondaries needs a CoreRef<M>&. I didn't use celeritas_gen_action as the extend functions are already written in terms of kernels, but maybe that's needed here, or there's a need for an "Action-with-mutable-input"?

[drbenmorgan]

Digging deeper, this either isn't feasible as written, or I'm missing a trick in how the collections and views work. The extend_from_... and initialize_tracks free functions ultimately call a non-const member function of the underlying collections, the canonical case being in extend_from_primaries, where data.initializers is
ResizableItems<TrackInitializer>:

celeritas/src/celeritas/track/TrackInitUtils.hh

Line 42 in 83e65dc

data.initializers.resize(data.initializers.size() + host_primaries.size());

It's the same resize function causing the problem in the other related functions. The kernel calls inside these functions are fine though as they take a CoreRef<M> const&. With resize being fundamentally non-const

celeritas/src/celeritas/track/TrackInitData.hh

Lines 104 to 109 in 83e65dc

	//! Change the size without changing capacity
	CELER_FUNCTION void resize(size_type size)
	{
	CELER_EXPECT(size <= this->capacity());
	count = size;
	}

I don't see an obvious way to easily Action-ify these functions without a const_cast or a more involved implementation.

[sethrj]

You know, this might be why I didn't do that earlier 😔 I think I'm ok with a const-cast for this one: we might consider moving to CoreParams const&, CoreState* from the single CoreRef const& (with the pointed-to state being implicitly modified). Can you try it out with the const-cast, and then we can think about refactoring the function call later?

You are correct that we shouldn't need any celeritas-gen shenanigans: the execute statement will be a bunch of C++ function calls (which in turn call kernels).

[drbenmorgan]

You know, this might be why I didn't do that earlier 😔 I think I'm ok with a const-cast for this one: we might consider moving to CoreParams const&, CoreState* from the single CoreRef const& (with the pointed-to state being implicitly modified). Can you try it out with the const-cast, and then we can think about refactoring the function call later?

O.k, done and submitted with a basic skeleton for the Actions for discussion. The other thing I noted in terms of impacts is how to count the active/alive/queued in Stepper::operator(), and who should create the Actions (e.g. in CoreParams like for a couple of these).

[drbenmorgan]

Just to start closing this out (though I realise it's not high priority!), the one remaining action is the extend_from_primaries. As outlined above, this is potentially a little more involved given it requires input of a Span<Primary const>, and there's no direct mechanism to supply anything other than a CoreRef<M> to Actions.

@sethrj, if I understood your earlier comment, the suggestion here is to add the primaries as a member of TrackInitStateData? I think that's do-able if we factor out the copy-Primaries-to-device part of extend_from_primaries:

    // Allocate memory and copy primaries
    Collection<Primary, Ownership::value, M> primaries; // This would become a member of TrackInitStateData
    resize(&primaries, host_primaries.size());
    Copier<Primary, MemSpace::host> copy{host_primaries};
    copy(M, primaries[AllItems<Primary, M>{}]);

into a separate function (not 100% sure on inclusion or not of the size/capacity checks and resize operations). The remainder would factor into a new extend_from_primaries(CoreRef<M> const&) and could still retain the existing signature for back compatibility as just called both in sequence.

Then Stepper<M>::operator()(SpanConstPrimary primaries) would just call the "copy to device" function, and the actual extension would be wrapped in an Action added to the default registry like the others.

Does that sound like a reasonable approach?