refactor: use ValueTask over Task where appropriate

[JamieMagee]

---

Before the change?

• WebhookEventProcessor method signatures return Task

After the change?

• WebhookEventProcessor method signatures return ValueTask

Pull request checklist

• Tests for the changes have been added (for bug fixes / features)
• Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

• Yes
• No

---

[github-actions]

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

[Copilot]

Pull Request Overview

This PR refactors the webhook processing API to use ValueTask instead of Task for better performance characteristics in scenarios where operations complete synchronously or use cached results. The primary motivation is to optimize allocations and improve performance for lightweight webhook processing operations.

Key Changes

• Replaced all Task return types with ValueTask in webhook processing methods
• Updated sample applications and documentation to reflect the new API
• Modified Azure Functions integration to use the updated async patterns

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/Octokit.Webhooks/WebhookEventProcessor.cs	Core refactoring of all webhook processing methods from Task to ValueTask
src/Octokit.Webhooks.AzureFunctions/GitHubWebhooksHttpFunction.cs	Updated Azure Functions integration to pass cancellation token
samples/AzureFunctions/MyWebhookEventProcessor.cs	Updated sample to use ValueTask return type
samples/AspNetCore/MyWebhookEventProcessor.cs	Updated sample to use ValueTask return type
README.md	Updated documentation examples and added new parameter documentation

[sommmen]

@JamieMagee nitpick: not to meddle, do your thing, but i kinda tend to lean towards keeping Task unless completely necessary;

https://devblogs.microsoft.com/dotnet/understanding-the-whys-whats-and-whens-of-valuetask/#should-every-new-asynchronous-api-return-valuetask-/-valuetask

As highlighted above, Task and Task are easier to use correctly than are ValueTask and ValueTask, and so unless the performance implications outweigh the usability implications, Task / Taskare still preferred. There are also some minor costs associated with returning a ValueTask instead of a Task, e.g. in microbenchmarks it’s a bit faster to await a Task than it is to await a ValueTask, so if you can use cached tasks (e.g. you’re API returns Task or Task), you might be better off performance-wise sticking with Task and Task. ValueTask / ValueTask are also multiple words in size, and so when these are awaitd and a field for them is stored in a calling async method’s state machine, they’ll take up a little more space in that state machine object.

(emaphasis mine)

I think the methods are always awaited, so this would cause less performance, and ValueTask isn't a common thing, people would know Task better.

[JamieMagee]

I think the methods are always awaited, so this would cause less performance, and ValueTask isn't a common thing, people would know Task better.

I think it's important to read that sentence as a whole, not in part:

in microbenchmarks it’s a bit faster to await a Task<TResult> than it is to await a ValueTask<TResult>, so if you can use cached tasks (e.g. you’re API returns Task or Task<bool>), you might be better off performance-wise sticking with Task and Task<bool>.

This isn't a microbenchmark, and these tasks can't be cached.

I think this is a good use case for ValueTask. Picking out some a different point from that article:

However, ValueTask / ValueTask<TResult> are great choices when a) you expect consumers of your API to only await them directly, b) allocation-related overhead is important to avoid for your API, and c) either you expect synchronous completion to be a very common case, or you’re able to effectively pool objects for use with asynchronous completion. When adding abstract, virtual, or interface methods, you also need to consider whether these situations will exist for overrides/implementations of that method.

a) We call WebhookEventProcessor in GitHubWebhookExtensions or GitHubWebhooksHttpFunction and we only ever directly await
b) A webhook API is potentially a high-throughput code path where performance is important
c) I expect the synchronous completion case to be the most common as ValueTask.CompletedTask is the default implementation and I only expect some methods to be overridden

But let me split out the ValueTask changes from the other changes.

[martincostello]

This change seems reasonable to me, particularly given that use of ValueTask should reduce the number of allocations made as part of serving requests.

[sommmen]

I think the methods are always awaited, so this would cause less performance, and ValueTask isn't a common thing, people would know Task better.

I think it's important to read that sentence as a whole, not in part:

in microbenchmarks it’s a bit faster to await a Task<TResult> than it is to await a ValueTask<TResult>, so if you can use cached tasks (e.g. you’re API returns Task or Task<bool>), you might be better off performance-wise sticking with Task and Task<bool>.

This isn't a microbenchmark, and these tasks can't be cached.

I think this is a good use case for ValueTask. Picking out some a different point from that article:

However, ValueTask / ValueTask<TResult> are great choices when a) you expect consumers of your API to only await them directly, b) allocation-related overhead is important to avoid for your API, and c) either you expect synchronous completion to be a very common case, or you’re able to effectively pool objects for use with asynchronous completion. When adding abstract, virtual, or interface methods, you also need to consider whether these situations will exist for overrides/implementations of that method.

a) We call WebhookEventProcessor in GitHubWebhookExtensions or GitHubWebhooksHttpFunction and we only ever directly await b) A webhook API is potentially a high-throughput code path where performance is important c) I expect the synchronous completion case to be the most common as ValueTask.CompletedTask is the default implementation and I only expect some methods to be overridden

But let me split out the ValueTask changes from the other changes.

You've won me over, sounds good!