Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

[2.1.1] Fix Json perf regression in Socket Transport #2578

Merged
halter73 merged 1 commit into from
May 31, 2018
Merged

[2.1.1] Fix Json perf regression in Socket Transport #2578

halter73 merged 1 commit into from
May 31, 2018

Conversation

@halter73
Copy link
Member

@halter73 halter73 commented May 16, 2018

#2573

This PR is based on dev for now. Should we start preparing a patch branch?

@davidfowl
Copy link
Member

davidfowl commented May 16, 2018

Yes

@sebastienros
Copy link
Member

sebastienros commented May 16, 2018

Did you measure or should I?

@halter73
Copy link
Member Author

halter73 commented May 16, 2018

@natemcmaster Should I create a release/2.1.1 branch based off of release/2.1 for this?

@sebastienros
Copy link
Member

sebastienros commented May 16, 2018

I built this branch locally and ran with and without this assembly in release on Linux. But it gets worse with this change at 261K vs 300K. We were almost at 320K before the regression.

@natemcmaster
Copy link
Contributor

natemcmaster commented May 16, 2018

Should I create a release/2.1.1 branch based off of release/2.1 for this?

You can use a branch named halter73/release/2.1.1 if you want to stage multiple 2.1.1 fixes, but don't add a new release/* branch. Those have special meaning to our CI, and are also the way we keep track of changes that have patch approval. Once we finish RTM, 2.1.1 fixes will go into release/2.1.

tmds
tmds reviewed May 17, 2018
View reviewed changes
src/Kestrel.Transport.Sockets/SocketTransport.cs Outdated
_numSchedulers = InlineSchedulerArray.Length;
_schedulers = InlineSchedulerArray;
}
else if (ioQueueCount > 0)
Copy link
Contributor

@tmds tmds May 17, 2018

Choose a reason for hiding this comment

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

This disable the ioQueues on Linux. Is this intentional?

Copy link
Member Author

@halter73 halter73 May 17, 2018

Choose a reason for hiding this comment

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

It was intentional, but but given that this PR regressed perf even further, it's likely we'll run Socket IO completion callbacks "inline" on Linux (since IO completion already gets dispatched to the ThreadPool) and go back to scheduling pipe continuations to the IOQueues.

@halter73 halter73 self-assigned this May 25, 2018
@halter73 halter73 removed their assignment May 25, 2018
@halter73 halter73 changed the base branch from dev to release/2.1 May 25, 2018 22:59
@halter73 halter73 added the SHP: Approval pending Shiproom approval is required for the issue label May 25, 2018
@halter73 halter73 changed the title Fix Json perf regression in Socket Transport [2.1.1] Fix Json perf regression in Socket Transport May 25, 2018
@sebastienros
Copy link
Member

sebastienros commented May 30, 2018

I verified the performance numbers and the fix reverted the regression.

@halter73 halter73 added SHP: Approved Shiproom has approved the issue and removed SHP: Approval pending Shiproom approval is required for the issue labels May 31, 2018
@halter73 halter73 merged commit e4d290b into release/2.1 May 31, 2018
@halter73 halter73 deleted the halter73/2573 branch May 31, 2018 23:28
@halter73 halter73 mentioned this pull request Feb 5, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@sebastienros sebastienros Awaiting requested review from sebastienros

2 more reviewers

@tmds tmds tmds left review comments

@davidfowl davidfowl davidfowl approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

SHP: Approved Shiproom has approved the issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@halter73 @davidfowl @sebastienros @natemcmaster @tmds