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

Change SslStream's internal adapter interface to use static abstract interface methods #65239

Merged
merged 2 commits into from
Feb 15, 2022

Conversation

stephentoub
Copy link
Member

Passing around the adapter instance was working around the lack of static abstract interface methods. Now that we have the latter, we can get rid of the former, and pass around just the data that's needed. This will make some of the state machines smaller, as we're now just accessing InnerStream rather than passing the stream around as part of the adapter. I also removed the isApm argument that was adding complication and expense purely to add a method name into an exception message thrown when the type is misused, but a) that information is already available in the resulting exception's stack trace, and b) it adds expense by increasing the size of the async method state machines.

cc: @wfurt, @tannergooding

@ghost
Copy link

ghost commented Feb 12, 2022

Tagging subscribers to this area: @dotnet/ncl, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Passing around the adapter instance was working around the lack of static abstract interface methods. Now that we have the latter, we can get rid of the former, and pass around just the data that's needed. This will make some of the state machines smaller, as we're now just accessing InnerStream rather than passing the stream around as part of the adapter. I also removed the isApm argument that was adding complication and expense purely to add a method name into an exception message thrown when the type is misused, but a) that information is already available in the resulting exception's stack trace, and b) it adds expense by increasing the size of the async method state machines.

cc: @wfurt, @tannergooding

Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Net.Security

Milestone: -

@stephentoub stephentoub force-pushed the securitystreamstaticinterfaces branch 2 times, most recently from b0e79c6 to e3de7fa Compare February 14, 2022 01:50
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM.
I assume that smaller state will not really be measurable with microbenchmarks?

@stephentoub
Copy link
Member Author

I assume that smaller state will not really be measurable with microbenchmarks?

Not really.

@stephentoub
Copy link
Member Author

@lambdageek, there are a bunch of mono failures here of the form "BadImageFormatException : Method has no body"... is this a sign of a problem with static abstract interface methods?

@geoffkizer
Copy link
Contributor

Pardon my ignorance, but how are static abstract methods actually implemented? Is there a design note somewhere or something like that?

@stephentoub
Copy link
Member Author

how are static abstract methods actually implemented? Is there a design note somewhere or something like that?

@davidwrighton, can you point @geoffkizer to any design docs you may have on this?

@lambdageek
Copy link
Member

@lambdageek, there are a bunch of mono failures here of the form "BadImageFormatException : Method has no body"... is this a sign of a problem with static abstract interface methods?

Yes that's suspicious.

@lambdageek
Copy link
Member

Standalone repro for the mono issue #65384

It's only used to add duplicative information to an exception message, and in doing so it makes the async methods it's used in more expensive.
@stephentoub stephentoub force-pushed the securitystreamstaticinterfaces branch from e3de7fa to eebc607 Compare February 15, 2022 16:26
@stephentoub stephentoub merged commit 10bf322 into dotnet:main Feb 15, 2022
@stephentoub stephentoub deleted the securitystreamstaticinterfaces branch February 15, 2022 20:33
@davidwrighton
Copy link
Member

@geoffkizer
Copy link
Contributor

@davidwrighton Interesting, thanks for the link. I had naively assumed that they were implemented through some sort of compiler transform.

@ghost ghost locked as resolved and limited conversation to collaborators Mar 19, 2022
@karelz karelz added this to the 7.0.0 milestone Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants