Skip to content

Conversation

@BobTheBuidler
Copy link
Contributor

This is a second option for #697 with the comments in that PR implemented here for separate review

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 7, 2025

CodSpeed Performance Report

Merging #708 will improve performances by ×560

Comparing BobTheBuidler:refactored (33e5dca) with master (3341173)

Summary

⚡ 6 improvements
✅ 33 untouched

Benchmarks breakdown

Benchmark BASE HEAD Change
test_internal_task_done_callback_microbenchmark[cancelled-func-bounded] 3.5 ms 1.6 ms ×2.2
test_internal_task_done_callback_microbenchmark[cancelled-func-unbounded] 3.5 ms 1.6 ms ×2.2
test_internal_task_done_callback_microbenchmark[exception-func-bounded] 960.4 ms 1.7 ms ×560
test_internal_task_done_callback_microbenchmark[exception-func-unbounded] 965.4 ms 1.7 ms ×560
test_internal_task_done_callback_microbenchmark[finished-func-bounded] 3.9 ms 1.8 ms ×2.1
test_internal_task_done_callback_microbenchmark[finished-func-unbounded] 3.9 ms 1.8 ms ×2.1

@BobTheBuidler
Copy link
Contributor Author

Yeah this is better than #697 , check out the performance gains that come with it

@codecov
Copy link

codecov bot commented Nov 7, 2025

Codecov Report

❌ Patch coverage is 97.14286% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.05%. Comparing base (3341173) to head (33e5dca).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
tests/test_cancel.py 93.93% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #708      +/-   ##
==========================================
- Coverage   97.22%   97.05%   -0.17%     
==========================================
  Files          12       13       +1     
  Lines         793      816      +23     
  Branches       41       42       +1     
==========================================
+ Hits          771      792      +21     
- Misses         20       22       +2     
  Partials        2        2              
Flag Coverage Δ
unit 95.71% <97.14%> (-0.13%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@property
def __tasks(self) -> List["asyncio.Task[_R]"]:
# NOTE: I don't think we need to form a set first here but not too sure we want it for guarantees
return list(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a list is probably overkill, we could just return an iterator for all the uses I saw.

{
cache_item.task
for cache_item in self.__cache.values()
if not cache_item.task.done()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the condition is needed or useful for the use cases I saw.

@Dreamsorcerer
Copy link
Member

I think we can merge the appropriate changes into the other PR, to keep reviewing in one place.

self.__closed = True

tasks = list(self.__tasks)
tasks = self.__tasks
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to materialize tasks into a list or set or some other container here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, that's already handled by the original code.

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.

2 participants