Massively optimizing TaskDriver performance #264
Conversation
| if (!m_ProgressLookupData.IsDataInvalidated && m_ParentProgressLookupData is | ||
| { | ||
| IsDataInvalidated: false | ||
| }) |
There was a problem hiding this comment.
just to double check, this is just syntax sugar. It doesn't resolve to some sort of reflection.
There was a problem hiding this comment.
Yeah it's just Rider merging the following into a pattern.
if (!m_ProgressLookupData.IsDataInvalidated && (m_ParentProgressLookupData != null && !m_ParentProgressLookupData.IsDataInvalidated))
I've never seen it do it like that before but there isn't any reflection or allocations going on.
There was a problem hiding this comment.
OK, just wanted to double check because I have seen that pattern matching style produce less efficient code
| /// <summary> | ||
| /// Whether the underlying data has potentially been updated by something getting write access to it. | ||
| /// </summary> | ||
| public virtual bool IsDataInvalidated | ||
| { | ||
| //If the current Read dependency has changed from what we last stored, then someone has written here | ||
| get => m_AccessController | ||
| .GetDependencyFor(AccessType.SharedRead) | ||
| .Equals_NoBox(m_LastSharedReadAccessJobHandle); | ||
| } | ||
|
|
There was a problem hiding this comment.
This only works if nobody has called AcquireAsync(AccessType.SharedRead) since write access was gained.
I think we'd be better off having the caller pass in the read handle at the last time they requested read because data invalidation is relative to the perspective of the data consumer not the provider.
There was a problem hiding this comment.
I'm not sure I fully understand. I've written out a flow below. Am I missing something?
I think we are handling this where AbstractData is the consumer and handles storing the read handle for the last time it requested read.
TDA tries to read from AbstractData
- LastSharedReadAccess set to the Data's Read Dependency (1)
TDB tries to read from same AbstractData
- LastSharedReadAccess set to the Data's Read Dependency (still 1)
TDC tries to write to the same AbstractData
- Data's dependencies move up the read is now (2)
TDD tries to read from the same AbstractData
- Data is invalid because the AbstractData's LastSharedReadAccess that was stored is not the same anymore.
- LastSharedReadAccess set to the Data's Read Dependency (now 2)
There was a problem hiding this comment.
After your last point what if TDA then tries to read from AbstractData?
It doesn't know that the data has changed since its last read and will think that IsDataInvalidated
From what I'm understanding, this results in one member's read operation hiding the data change from all other potential readers. It's only the first member that reads after a write that gets to know that the data is invalidated.
There was a problem hiding this comment.
Ugh... you're right!
Good point. I've moved the storage and checking of the read handles to where the caller for the jobs are which makes sense. We really want to know if we've been written to since the last time we ran this specific job. If we have, then our job needs to run again. If we haven't, then nothing has changed and so we don't need to run our job.
# Conflicts: # Scripts/Runtime/Entities/TaskDriver/TaskSet/TaskData/DataStream/DataSource/Data/ActiveArrayData.cs
|
@mbaker3 Ready for re-review |
...ntime/Entities/TaskDriver/TaskSet/TaskData/DataStream/Cancellation/CancelProgressFlowNode.cs
Show resolved
Hide resolved
Optimized TaskDrivers so they won't schedule jobs if no one has written to them.
What is the current behaviour?
TaskDrivers always run their jobs even if there is no data to operate on. This leads to a lot of scheduling overhead that adds up.
What is the new behaviour?
TaskDriverManagementSystemforces a sync after consolidation. This is to prevent issues with circular writing patterns such as:By forcing the consolidation, we can also check the length to ensure we actually have data when scheduling.
AbstractDataexposes a boolean to let callers know if its data could have been invalidated. If so, it's worth scheduling the job. Otherwise, no one has touched it.AbstractJobConfigwill only schedule jobs if the scheduling data has data to work on.AbstractDataSourcewill only consolidate data if there has data to work on.What issues does this resolve?
What PRs does this depend on?
Does this introduce a breaking change?