search

aspnet / KestrelHttpServer

[Archived] A cross platform web server for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
2.62k stars 526 forks source link

[2.2] Use non-pinnable buffer for zero byte read #3094

Closed benaadams closed 6 years ago

benaadams commented 6 years ago

Time is being spent Pinning

image

and Unpinning

image

Kestrel uses prepinned buffers so shouldn't have these costs. It comes from pinning Array.Empty<byte>() in the zero-byte reads.

After using Memory<byte>.Empty these costs go away:

image

@sebastienros could you benchmark this; see if it makes a difference?

sebastienros commented 6 years ago

Plaintext (pipelined) not impacted by this change.

Json goes back to the levels of 2.1, so actually reverting the negative impact of the ZBR PR. DbFortunesEf is slightly better.

benaadams commented 6 years ago

Marked as [2.2] as it targeting that branch

Eilon commented 6 years ago

Moving to patch milestone.

muratg commented 6 years ago

@Eilon @natemcmaster I'm assuming it's fine to check-in to release/2.2 now right? (We're aiming to move Kestrel to mondorepo ASAP so it would be good to clear the PRs ASAP.

Tratcher commented 6 years ago

It's too late to clear PRs, we'll need to re-do them in the mondo repo.

natemcmaster commented 6 years ago

If you merge this in the next few hours, it's fine to go into release/2.2. But you're racing against https://github.com/aspnet/AspNetCore/pull/4026. Once https://github.com/aspnet/AspNetCore/pull/4026 is merged, I'll be locking release/2.2 and master from any changes.

halter73 commented 6 years ago

It's in.

sebastienros commented 6 years ago

I get it now, this time window only allowed for this PR not to be lost when you move to mondo-repo, and it will be part of the next servicing release for 2.2.

Eilon commented 6 years ago

This... wasn't approved for 2.2...

Eilon commented 6 years ago

@halter73 / @muratg - this change isn't approved for 2.2 or 2.2.1 yet. Can we please back it out until it's approved?

muratg commented 6 years ago

My apologies, this was my mistake. I guess I "imagined" that this PR was sent to master branch.

Eilon is right. This is OK to check-in to master, but for release/2.2.1 we need to get shiproom approval.

@halter73 could you revert it please? And we can talk offline to decide if we want to take this to shiproom.

halter73 commented 6 years ago

Reverted.

Eilon commented 6 years ago

Thanks! We should keep an open PR though so that we can take it through shiproom. Unfortunately can't re-open a PR. Can we send a new one from the same fork or something?

Tratcher commented 6 years ago

We're moving to mondo-repo now, we'll need to create a new PR over there.

benaadams commented 6 years ago

mondo-repo? Is that 'cos mono-repo would be confusing in the .NET world?

Or am I just not up to date with mah lingo?

Tratcher commented 6 years ago

@benaadams see https://github.com/aspnet/AspNetCore/pull/4026

benaadams commented 6 years ago

We're moving to mondo-repo now, we'll need to create a new PR over there.

In which branch? (master, release2.2 etc)

Tratcher commented 6 years ago

2.1 just moved, 2.2 is in flight, master is next.

natemcmaster commented 6 years ago

2.2 - https://github.com/aspnet/AspNetCore/pull/4031

davidfowl commented 6 years ago

@halter73 we should get this into master

benaadams commented 6 years ago

Zero byte read for AspNetCore master https://github.com/aspnet/AspNetCore/pull/4059

vivmishra commented 6 years ago

Approved for 2.2.1