Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Aggressive Inline fast-path of BuffersExtensions split methods#28928

Merged
ahsonkhan merged 1 commit into
dotnet:masterfrom
benaadams:buffersextensions
Apr 11, 2018
Merged

Aggressive Inline fast-path of BuffersExtensions split methods#28928
ahsonkhan merged 1 commit into
dotnet:masterfrom
benaadams:buffersextensions

Conversation

@benaadams
Copy link
Copy Markdown
Member

@benaadams benaadams commented Apr 8, 2018

Methods are split into a fast-path inline part and a slower non-inline part. Aggressively inline the fast-path.

from aspnet/SignalR#1907 (comment)

/cc @ahsonkhan @pakrym @davidfowl @JamesNK

@benaadams
Copy link
Copy Markdown
Member Author

Hopefully this will also allow the Jit to devirtualize the IBufferWriter<T> for Write<T> on a sealed class

/// Writes contents of <paramref name="value"/> to <paramref name="writer"/>
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static void Write<T>(this IBufferWriter<T> writer, ReadOnlySpan<T> value)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is an interface call..

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Its a non-virtual static call? A static method can't implement an interface

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

And hopefully when its inlined and called with the sealed class MemoryBufferWriter the Jit will devirtalize it as the interface call inside cannot be any other type? /cc @AndyAyersMS

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

May not be able to look at this for a few days, but superficially yes sealing the class should be sufficient.

@pakrym
Copy link
Copy Markdown

pakrym commented Apr 9, 2018

Should we start by adding no inline to slower methods and see if JIT can figure it out?

@benaadams
Copy link
Copy Markdown
Member Author

Should we start by adding no inline to slower methods and see if JIT can figure it out?

They shouldn't be inlined; I think it rejects anything with a loop as an inline candidate if it doesn't have AggressiveInline

@ahsonkhan
Copy link
Copy Markdown

@dotnet-bot test this please

@ahsonkhan ahsonkhan merged commit af8ca4f into dotnet:master Apr 11, 2018
@karelz karelz added this to the 2.1.0 milestone Apr 12, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 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