Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Task to ValueTask (#988) #1964

Merged
merged 2 commits into from
Apr 10, 2023
Merged

Conversation

jafin
Copy link
Contributor

@jafin jafin commented Mar 24, 2023

Refactor usages of Task with ValueTask

@drieseng
Copy link
Contributor

@jafin, @lahma: I'm sure you'll burn me for this, but ... should we consider removing use of async (overhead) for methods that are only ever invoked in a "blocking kind of sense" from a dedicated (QuartzScheduler)thread?

@jafin
Copy link
Contributor Author

jafin commented Mar 29, 2023

@drieseng I don't have an opinion really, what you suggest sounds like an improved position and you're more aware of the overhead gains.

Should it be a separate PR to this?

Concerning overheads, I ran the existing benchmarks in quartz before and after this PR change
(not really taking an interest in what they covered) and the results from task -> valuetask appeared to be insignificant in performance improvements. From my reading, unless there are serious hot paths with tasks, this aligns with expectations.

(Or perhaps there are improvements to be made to this PR?)

Perhaps the benchmarks don't cover all the hot paths where changes like this would make a difference 🤷🏻

EDIT: Also happy to take suggestions to improve this PR if there are things that should be changed. 👍🏻

@lahma lahma merged commit 28b5fae into quartznet:main Apr 10, 2023
@lahma
Copy link
Member

lahma commented Apr 10, 2023

Thanks @jafin ! this is great groundwork for changing the APIs, I converted some bits I saw missing (might be more) but this will help to convert missing bits in smaller increments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants