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

Cancellable: remove UsingToken usages in tests #18276

Merged
merged 3 commits into from
Feb 4, 2025

Conversation

auduchinok
Copy link
Member

Removes Cancellable.UsingToken from the public surface, since the computation now always sets the token.

@auduchinok auduchinok requested a review from a team as a code owner January 28, 2025 04:36
Copy link
Contributor

github-actions bot commented Jan 28, 2025

✅ No release notes required

Copy link
Member Author

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

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

Accidental comment from the release notes editor, I don't seem to be able to delete it.

@auduchinok
Copy link
Member Author

So, just removing the member from the public API didn't work, as it was used in Cancellable.run which is marked inline. Removing that member produced errors about insufficiently public member being used in an inline member. I've removed the inline. @majocha Could you help me with the performance testing of this change, please?

@auduchinok auduchinok changed the title Remove Cancellable.UsingToken Cancellable: remove UsingToken Jan 28, 2025
@majocha
Copy link
Contributor

majocha commented Jan 28, 2025

@auduchinok I'm not sure about removing inline there, would it affect stack traces and debugging?
Possibly it would be better to remove use _ = Cancellable.UsingToken(ct) from run and only set the token in toAsync instead.

Somewhat related: It would be great to integrate the functionality of GuardCancellable into the cancellable CE and thus simplify the stuff that's going on in CheckDeclarations.fs

@auduchinok auduchinok changed the title Cancellable: remove UsingToken Cancellable: remove UsingToken in tests Jan 30, 2025
@auduchinok auduchinok changed the title Cancellable: remove UsingToken in tests Cancellable: remove UsingToken usages in tests Jan 30, 2025
@auduchinok auduchinok force-pushed the cancellable-simplifyTests branch from 54bf644 to f41696e Compare January 30, 2025 09:39
auduchinok and others added 2 commits January 30, 2025 10:49
The token is now always set inside the cancellable computation
@auduchinok auduchinok force-pushed the cancellable-simplifyTests branch from f41696e to 91ef654 Compare January 30, 2025 09:49
@auduchinok
Copy link
Member Author

auduchinok commented Jan 30, 2025

@majocha I've reverted the changes around UsingToken and run, as you experiment with them in #18285. This PR is now only about simplifying the tests.

@majocha
Copy link
Contributor

majocha commented Jan 30, 2025

So, I did some benchmark runs using FCSSourceFiles benchmark and frankly I see no difference between inlined and not inlined Cancellable.run. Looking at the original implementation:

    /// Run a cancellable computation using the given cancellation token
    let inline run (ct: CancellationToken) (Cancellable oper) =
        if ct.IsCancellationRequested then
            ValueOrCancelled.Cancelled(OperationCanceledException ct)
        else
            oper ct

it seems the idea was to just expand the cancel check inline.

@psfinaki
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki psfinaki merged commit 1f9b0ce into dotnet:main Feb 4, 2025
33 checks passed
@auduchinok auduchinok deleted the cancellable-simplifyTests branch February 4, 2025 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants