Skip to content

Cancellable: remove UsingToken usages in tests - #18276

Merged
psfinaki merged 3 commits into
dotnet:mainfrom
auduchinok:cancellable-simplifyTests
Feb 4, 2025
Merged

Cancellable: remove UsingToken usages in tests#18276
psfinaki merged 3 commits into
dotnet:mainfrom
auduchinok:cancellable-simplifyTests

Conversation

@auduchinok

Copy link
Copy Markdown
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 ownerJanuary 28, 2025 04:36
@github-actions

github-actionsBot commented Jan 28, 2025

Copy link
Copy Markdown
Contributor

✅ No release notes required

@auduchinokauduchinok left a comment

Copy link
Copy Markdown
MemberAuthor

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
Copy Markdown
MemberAuthor

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?

@auduchinokauduchinok changed the title Remove Cancellable.UsingTokenCancellable: remove UsingTokenJan 28, 2025
@majocha

Copy link
Copy Markdown
Contributor

@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

@auduchinokauduchinok changed the title Cancellable: remove UsingTokenCancellable: remove UsingToken in testsJan 30, 2025
@auduchinokauduchinok changed the title Cancellable: remove UsingToken in testsCancellable: remove UsingToken usages in testsJan 30, 2025
@auduchinok
auduchinokforce-pushed the cancellable-simplifyTests branch from 54bf644 to f41696eCompareJanuary 30, 2025 09:39
auduchinokand others added 2 commits January 30, 2025 10:49
The token is now always set inside the cancellable computation
@auduchinok
auduchinokforce-pushed the cancellable-simplifyTests branch from f41696e to 91ef654CompareJanuary 30, 2025 09:49
@auduchinok

auduchinok commented Jan 30, 2025

Copy link
Copy Markdown
MemberAuthor

@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
Copy Markdown
Contributor

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 tokenlet inlinerun(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
Copy Markdown
Contributor

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
psfinaki merged commit 1f9b0ce into dotnet:mainFeb 4, 2025
@auduchinok
auduchinok deleted the cancellable-simplifyTests branch February 4, 2025 12:06
Sign up for freeto 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

@auduchinok@majocha@psfinaki@KevinRansom