Switch to value-tasks in a few oop syncing points. #63056
Conversation
| { | ||
| Debug.Assert(!checksums.Contains(Checksum.Null)); | ||
| if (checksums.Count == 0) | ||
| return; |
There was a problem hiding this comment.
this is a core change/thing i noticed. When logging this information. we call into this tons of times with nothing to actually sync. Roughly 98%+ of the time this happens. so this can insta-bail. Which allows this to sensibly be a value task. the rest is just bubbling that upwards for all calls that themselves are just calling awaiting value-task helpers.
| @@ -52,24 +52,22 @@ public override async Task<T> GetAssetAsync<T>(Checksum checksum, CancellationTo | |||
| } | |||
| } | |||
|
|
|||
| public async Task<List<ValueTuple<Checksum, T>>> GetAssetsAsync<T>(IEnumerable<Checksum> checksums, CancellationToken cancellationToken) | |||
| public async ValueTask<ImmutableArray<ValueTuple<Checksum, T>>> GetAssetsAsync<T>(HashSet<Checksum> checksums, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
passing items with known length allows us to optimize more (both by bailing out early and by getting the right sized immutable collections to return).
There was a problem hiding this comment.
Should we use an Immutable type here?
CyrusNajmabadi
force-pushed
the
syncOverhead
branch
from
b130bfa to
658dad5
Compare
CyrusNajmabadi
force-pushed
the
syncOverhead
branch
from
658dad5 to
72c22c5
Compare
CyrusNajmabadi
force-pushed
the
syncOverhead
branch
from
accc866 to
77180d9
Compare
| public override Task<T> GetAssetAsync<T>(Checksum checksum, CancellationToken cancellationToken) | ||
| => _validator.GetValueAsync<T>(checksum); | ||
| public override async ValueTask<T> GetAssetAsync<T>(Checksum checksum, CancellationToken cancellationToken) | ||
| => await _validator.GetValueAsync<T>(checksum).ConfigureAwait(false); |
There was a problem hiding this comment.
❓why do we await here instead of just returning the value task?
There was a problem hiding this comment.
because _validator.GetValueAsync returns a Task, nto a ValueTask. This is a simple way to map between them.
| @@ -52,24 +52,22 @@ public override async Task<T> GetAssetAsync<T>(Checksum checksum, CancellationTo | |||
| } | |||
| } | |||
|
|
|||
| public async Task<List<ValueTuple<Checksum, T>>> GetAssetsAsync<T>(IEnumerable<Checksum> checksums, CancellationToken cancellationToken) | |||
| public async ValueTask<ImmutableArray<ValueTuple<Checksum, T>>> GetAssetsAsync<T>(HashSet<Checksum> checksums, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
Should we use an Immutable type here?
On the input side, no. THese are effectively 'scratch' sets where we're computing work to do, doing the work, then returning the scratch sets to the pool. Think of them as the 'set' equivalent to passing around work with a temporary ArrayBuilder. We want to avoid the unnecessary conversion of this to an immutable array as we're not ever storing this. |
ghost
added this to the 
Noticed while profiling this area.
I recommend reviewing one commit at a time.