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

Remove will_wake calls #1037

Merged
merged 2 commits into from
May 3, 2024
Merged

Conversation

akoshelev
Copy link
Collaborator

We used will_wake in correctness checks to make sure we don't hang the future/stream forever if a wrong waker is used, but the contract for this function is best-effort and it started generating more and more false positives.

The recent one, found by @aleiserson, is inside rust-lang (rust-lang/rust#119863), so it is very hard to get around.

We don't have a good replacement for this check unless we implement our own waker, but it is probably too much for now

We used `will_wake` in correctness checks to make sure we don't hang the future/stream forever if a wrong waker is used, but the contract for this function is best-effort and it started generating more and more false positives.

The recent one, found by @aleiserson, is inside rust-lang (rust-lang/rust#119863), so it is very hard to get around.

We don't have a good replacement for this check unless we implement our own waker, but it is probably too much for now
Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 95.65217% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 90.59%. Comparing base (20c89ed) to head (56959cf).

Files Patch % Lines
...pa-core/src/helpers/transport/stream/collection.rs 90.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1037   +/-   ##
=======================================
  Coverage   90.58%   90.59%           
=======================================
  Files         173      173           
  Lines       26089    26082    -7     
=======================================
- Hits        23632    23628    -4     
+ Misses       2457     2454    -3     

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

Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

That's unfortunate. I guess the real expectation is that we maintain multiple wakers. That seems like a language feature that is really a bug: in that it imposes (unreasonable) costs on those using the API.

@akoshelev
Copy link
Collaborator Author

That's unfortunate. I guess the real expectation is that we maintain multiple wakers. That seems like a language feature that is really a bug: in that it imposes (unreasonable) costs on those using the API.

yea I am not sure I understand the motivation for making it best-effort, probably some hard constraint on the scheduling side. Or it was never designed to avoid false negatives, they were really only interested in true positives.

@akoshelev
Copy link
Collaborator Author

related to #1036

Copy link
Collaborator

@andyleiserson andyleiserson left a comment

Choose a reason for hiding this comment

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

There is also a comment around line 58 of ordering_sender.rs "here used to be a check that new waker will wake the same task", that can probably be deleted. (This PR and the linked issue seem like sufficient documentation of the demise of will_wake checks.)

@akoshelev
Copy link
Collaborator Author

There is also a comment around line 58 of ordering_sender.rs "here used to be a check that new waker will wake the same task", that can probably be deleted. (This PR and the linked issue seem like sufficient documentation of the demise of will_wake checks.)

yea I was contemplating about it and was really on the fence about that one. Now the balance is changed so I'll remove it

Use `clone_from` for micro optimization if we can
@akoshelev akoshelev merged commit dfe14ff into private-attribution:main May 3, 2024
11 checks passed
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.

3 participants