Task Driver - Improve Dependency Resolution and Fulfillment in the Job

[mbaker3]

NOTE: This task has changed purpose somewhat as of https://github.com/decline-cookies/anvil-unity-dots/issues/224#issuecomment-1644899738

---

Currently, task drivers key their data streams on type alone. This is a good default behaviour but there are situations where multiple streams of the same type are required.

For example, a task driver that supports a cancellable and non-cancellable request stream.

Today, the workaround is to create separate types either through private subtypes or completely separate types.

Provide a way to register multiple data streams of the same type.

Example Workaround

public readonly struct MyRequest
{
    private interface IMyRequest : IEntityProxyInstance
    {
        public MyRequest Request { get; }
    }

    public readonly struct NonCancellable : IMyRequest
    {
        public MyRequest Request { get; }

        public Entity Entity
        {
            get => Request.Entity;
        }

        public NonCancellable(in MyRequest request)
        {
            Request = request;
        }
    }

    public readonly struct Cancellable : IMyRequest
    {
        public MyRequest Request { get; }

        public Entity Entity
        {
            get => Request.Entity;
        }

        public Cancellable(in MyRequest request)
        {
            Request = request;
        }
    }

    public readonly int SomeVal;
    public Entity Entity { get; }

    public MyRequest(Entity requestingEntity, int someVal)
    {
        Entity = requestingEntity;
        SomeVal = someVal;
    }

    public static implicit operator Cancellable(in MyRequest request) => new Cancellable(in request);
    public static implicit operator NonCancellable(in MyRequest request) => new NonCancellable(in request);
}

[jkeon]

I'm not sure we want to do this.

If we do, we would have to introduce the concept of an ID for each DataStream.

Which could be user provided:

StartDataStream = CreateDriverDataStream<FollowPathStart>(IDs.FOLLOW_PATH_START);

Or generated and returned:

//Could return id and out the stream instead or you just get it off the stream
StartDataStream = CreateDriverDataStream<FollowPathStart>(out uint id);

But then we would have to be explicit on all the usages of that in job scheduling:

 actorMoveTaskDriver.ConfigureDriverJobTriggeredBy(
                    IDs.FOLLOW_PATH_START,
                    StartJob.Schedule,
                    BatchStrategy.MaximizeChunk)
                .RequireEntityPersistentDataForRead(FollowPathBufferEntityPersistentData)
                .RequireEntityPersistentDataForWrite(FollowPathStatusEntityPersistentData)
                .RequireDataStreamForWrite(actorMoveTaskDriver.GetDataStreamByID(IDs.MOVE_START));

Which might be fine but we lose out on some direct referencing and type safety. (Although maybe we could make that nicer by having the ID be contained inside the DataStream so you still direct reference but then pass in an ID to get an inner sibling collection)

But... the main concern is that Unity itself only keys on Type for all of ECS. If you need to have multiples of a type, then you wrap the struct in a different struct type which is what we can do as well for TaskDrivers.

Which is annoying, I agree, but just raising the question of whether we should do this or not?

[mbaker3]

I'm not sure about that last code block there. Why would we need the IDs? We have reference to the actual data stream instance. It's possible that the IDs are completely internal to TaskDriver so I can create two data streams of the same type and behind the scenes they resolve to their individual IDs.

I suppose there would be a limitation on the job side when you try to fetch the instance through the jobdata instance. It could work so long as you only want one of the streams in your job.

What if we provided the task driver instance in the job's constructor? Then you can fetch using the stream instance directly.

But... the main concern is that Unity itself only keys on Type for all of ECS. If you need to have multiples of a type, then you wrap the struct in a different struct type which is what we can do as well for TaskDrivers.

Just so I understand, is this a consistency with ECS concern or a limitation of the safety system and the way we manage job handles with Task Drivers?

[jkeon]

I'm not sure about that last code block there. Why would we need the IDs? We have reference to the actual data stream instance. It's possible that the IDs are completely internal to TaskDriver so I can create two data streams of the same type and behind the scenes they resolve to their individual IDs.

I suppose there would be a limitation on the job side when you try to fetch the instance through the jobdata instance. It could work so long as you only want one of the streams in your job.

What if we provided the task driver instance in the job's constructor? Then you can fetch using the stream instance directly.

Hmmn, yeah that's interesting. I could see that working. I wonder if you even need the require/get flow then...

The whole point before was that you needed to say what data you would need so we could make sure we got all the dependencies in order for you, then construct your job and give you an easy way to grab the data for your job and catch any issues where you hadn't requested access for it. And then we schedule.

But if we give the typed TaskDriver instance to your job, then the constructor is still main thread land before scheduling. Which means you we probably could:

1. Create the instance and pass in the TaskDriver.
2. Job Constructor grabs everything it needs off the TaskDriver via Require-like functions that handle getting the access.
3. Job then gets scheduled using all the access that was asked for in the constructor.

Instead of this:

            private StartJob(DataStreamJobData<FollowPathStart> jobData)
            {
                m_PathBuffers = jobData.GetEntityPersistentDataReader<FollowPathBuffer>();
                m_FollowPathStatusWriter = jobData.GetEntityPersistentDataWriter<FollowPathStatus>();
                m_ActorMoveWriter = jobData.GetDataStreamWriter<ActorMove>();
                m_VerboseLogger = new SXVerboseBurstableLogger<FixedString64Bytes>(typeof(StartJob).ToMessagePrefix());
            }

You'd get this:

            private StartJob(FollowPathTaskDriver taskDriver)
            {
                m_PathBuffers = taskDriver.RequireForRead(taskDriver.FollowPathBufferEntityPersistentData);
                m_FollowPathStatusWriter = taskDriver.RequireForWrite(taskDriver.FollowPathStatusEntityPersistentData);
                m_ActorMoveWriter = taskDriver.RequireForWrite(taskDriver.MoveDataStream);
                m_VerboseLogger = new SXVerboseBurstableLogger<FixedString64Bytes>(typeof(StartJob).ToMessagePrefix());
            }

That would eliminate the need for the method chaining require blocks

actorMoveTaskDriver.ConfigureDriverJobTriggeredBy(
                    StartDataStream,
                    StartJob.Schedule,
                    BatchStrategy.MaximizeChunk)
                .RequireEntityPersistentDataForRead(FollowPathBufferEntityPersistentData)
                .RequireEntityPersistentDataForWrite(FollowPathStatusEntityPersistentData)
                .RequireDataStreamForWrite(actorMoveTaskDriver.MoveDataStream);

Becomes:

actorMoveTaskDriver.ConfigureDriverJobTriggeredBy(
                    StartDataStream,
                    StartJob.Schedule,
                    BatchStrategy.MaximizeChunk);

We would probably want to make sure that API wise we've guarded against getting the end data without going through a special Require method.

This would be an annoying bug to track down since it MIGHT still work and just be a race condition:

            private StartJob(FollowPathTaskDriver taskDriver)
            {
                m_PathBuffers = taskDriver.RequireForRead(taskDriver.FollowPathBufferEntityPersistentData);
                m_FollowPathStatusWriter = taskDriver.RequireForWrite(taskDriver.FollowPathStatusEntityPersistentData);
               //OOPSIE DOOPSIE
                m_ActorMoveWriter = taskDriver.MoveDataStream.AcquireWriterAsync();
                m_VerboseLogger = new SXVerboseBurstableLogger<FixedString64Bytes>(typeof(StartJob).ToMessagePrefix());
            }

Just so I understand, is this a consistency with ECS concern or a limitation of the safety system and the way we manage job handles with Task Drivers?

Yeah just a consistency with ECS and a "if they restrict that way, what do they know that we don't." However in seeing your ideas above and thinking more about it, I've switched sides and I'm for this idea now.

[mbaker3]

Nice, yea that's interesting. Maybe a bit more refinement on how exactly you get references in the constructor but this is an interesting direction to pursue some day.

Maybe we let people do the actual acquire in the constructor? So long as they get the job handles out the caller of the constructor we'd be fine right? Rather than "Require" it could be something like:

taskDriver.AddDependency(MoveDataStream.AcquireWriterAsync(out m_ActorMoveWriter));

or even a required out param on the constructor that was some sort of dependency collection instance.

[jkeon]

Yeah I think we're saying the same thing:

My example of: m_ActorMoveWriter = taskDriver.RequireForWrite(taskDriver.MoveDataStream);

Would do what you said under the hood: taskDriver.AddDependency(MoveDataStream.AcquireWriterAsync(out m_ActorMoveWriter));

Names of course all being subject to change for better clarity etc.

The Taskdriver internally would already know about the Dependency collection for the job it was scheduling because it just instantiated you and is about to call schedule on it. So we don't need the developer to be explicit about it, we can handle it for them.

The explicit AcquireWriterAsync could also be hidden inside just so you don't see a bunch of Acquires without the corresponding Releases because we'd handle all the releases for you internally like we do today.

[mbaker3]

oh I see. Yea good point about the auto handling of release. I was just trying to avoid all the annoying wrapper types to maintain. But I think that may be settled now anyway

[mbaker3]

Changed the issue title to better reflect the work that needs to be done. Particularly after #245 gets closed with the workaround solution that will land soon.

Lots of discussion has happened around what we'd like to support some day. Here are some high level thoughts:

• We should consider a more dependency injection like approach where the current jobConfig.Request/Require methods setup the instance mappings and reflection is used to map the instances to the job fields automatically.
  • In cases where there are two dependencies/fields of the same type an [InjectionName("MyName")] can be used to resolve which dependency goes where.
  • This injection should be recursive through job parts.
  • Any post injection work that needs to be done (IE: Not part of Execute but needs reference to the injected fields (writers, CDFE, etc..) can be done in a method decorated with a [PostInjection] attribute.
• Alternatively, we can get rid of the jobConfig.Request/Require calls and produce a mapping in the job's constructor.
  • A reference to the host TaskDriver would be passed into the job's constructor just as it is with AddRequirementsFrom/ConfigureRequirements methods today.
  • The mapping would likely need three entries:
    • Destination field (on the job)
    • Source instance (data stream, CDFE type, etc...)
    • Usage (Write, Read, etc..) - We could maybe infer this from the destination field 🤷
  • The suitability of this approach will likely be influenced by the results of #243. If we go this route then an expensive constructor isn't a big deal.