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

net: use non-blocking syscall.Syscall for non-blocking calls #3412

Closed
rsc opened this issue Mar 27, 2012 · 23 comments

Comments

Projects
None yet
8 participants
@rsc
Copy link
Contributor

commented Mar 27, 2012

Looking at a real network server with 1000s of 
connections, there are 200+ threads created,
overwhelmingly for these calls from package net:

syscall.SetNonblock
syscall.SetsockoptInt
syscall.Write
syscall.Connect

All of these are, I believe "non-blocking", even Write.
We should probably make SetNonblock and SetsockoptInt be sysnb
by default, and maybe introduce syscall.WriteNB and syscall.ConnectNB.

Or else we should solve the 'blocking vs not' system call issue a
different way.
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2012

Comment 1:

Sounds good.  Just notes.  SetNonblock calls fcntl, which can block in some cases, so it
would need to call fcntlNB.  And it's hard to convince myself that setsockopt never
blocks, since it has so many different options at different kernel layers, so I think it
would be best to use SetsockoptIntNB as well.
@remyoudompheng

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2012

Comment 2:

On linux, setsockopt will trigger a buffer flush using TCP-NODELAY, so I believe it
blocks here.
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2012

Comment 3:

A member on the golang-nuts mailing list posted this profile today. I believe
implementing this, even partially, for Write, would improve the reported hello http
benchmark significantly.

Attachments:

  1. go_web_server.png (160681 bytes)
@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 16, 2012

Comment 4:

As long as you keep the existing syscall.Write as blocking and only use a new
syscall.WriteNB from code which guarantees the fd is in non-blocking mode (e.g. the net
package).
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2012

Comment 5:

Owner changed to @davecheney.

Status changed to Started.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Oct 21, 2012

Comment 6:

Patch set http://golang.org/cl/6739043/#ps2001 (not for submission) was
reported to give a 10% improvement by a user on the golang-nuts ML.
@remyoudompheng

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2012

Comment 7:

Sendmsg should be handled too. And the Read part too.
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2012

@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2012

Comment 9:

Labels changed: added size-l.

@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2013

Comment 10:

Hello,
After investigation making this change from Write to WriteNB does not generate a marked
improvement so I feel that this feature is not currently worth the cost and complexity
of its implementation. 
I have moved the ticket to thinking state and deprioritised the issue to Go1.1 Maybe.
Please correct that status if you feel this is incorrect.
Hopefully, with improvements to the scheduler this feature can be revisited with more
positive results.

Labels changed: added go1.1maybe, removed go1.1.

Owner changed to ---.

Status changed to Thinking.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 21, 2013

Comment 11 by songofacandy:

Attached patch makes "Hello World" webapp from 6200req/sec to 7300req/sec on Linux 3.2.0.
(ab -c10 -n20000, go 1.0.3)
Summary of this patch:
* Use accept4 and remove ForkLock
* Use WriteNB and ReadNB
* Currently, only for Linux
Could anyone verify the performance improvement of this patch?

Attachments:

  1. accept4.patch (7721 bytes)
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2013

Comment 12:

Can you please use a different tool to ab, its use has been discredited. Siege is a good
choice, or httpperf if you can get it to work.
@gopherbot

This comment has been minimized.

Copy link

commented Jan 21, 2013

Comment 13 by songofacandy:

command: siege -b -t 10S http://10.10.2.26:8888/
version: SIEGE 2.70
concurrency: 15
w/o GOMAXPROCS
w/o patch 4684.35 trans/sec
with patch 5572.23 trans/sec
with GOMAXPROCS=4
w/o patch 8368.84 trans/sec
with patch 9235.72 trans/sec
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2013

Comment 14:

sonofacandy: I think some of the improvements are the accept4 change, which can probably
be submitted independently.
This project uses Rietveld for code review. Can you please spin the accept4 change off
into a separate CL. You will find the contribution guidelines and instructions here,
http://golang.org/doc/contribute.html
@gopherbot

This comment has been minimized.

Copy link

commented Jan 22, 2013

Comment 15 by sebastien.paolacci:

I would second Dave's opinion. Accept4() make a lot of sense, and requiring a 2.6.28+
kernel shouldn't be a big issue.
Various tests tend to show that {read,write}NB do not (currently) bring significant
improvements (and possibly slow things down), so I would strip them out of a first patch.
Your current patch doesn't apply cleanly (e.g fd.go doesn't exists anymore): would you
mind just rebasing on tip first (without any refactoring) so we can run further
benchmarks?
Thanks,
Sebastien
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2013

Comment 16:

We have either a list of minimum platform specs, or an issue to create such a list for
go 1.1. From memory we require .25 or .27 on intel, but in practice for rhel and Debian
it's actually .32 as that is what ships in their current stable kernels, so I don't
think this is a big issue. 
It will mean Rhel5 moves from unsupported to broken which is a shame, unless we have
some kind of fallback if the syscall fails. I think there is an example of that in the
net package already.
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2013

Comment 17:

Re the patch, please move to the Rietveld system.
@gopherbot

This comment has been minimized.

Copy link

commented Jan 22, 2013

Comment 18 by songofacandy:

I've created https://golang.org/cl/7188044/
@davecheney

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2013

Comment 19:

Done forget to hg mail 7188044
@gopherbot

This comment has been minimized.

Copy link

commented Jan 22, 2013

Comment 20 by songofacandy:

I did it. Thank you.
@mreiferson

This comment has been minimized.

Copy link

commented Jan 22, 2013

Comment 21:

+1 to not breaking RHEL 5 unless it's absolutely necessary, a fallback if the syscall
fails would be fantastic.
@rsc

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2013

Comment 22:

[The time for maybe has passed.]

Labels changed: removed go1.1maybe.

@dvyukov

This comment has been minimized.

Copy link
Member

commented Jul 15, 2013

Comment 23:

I think it can be closed now. The new scheduler has some basic intelligence to determine
non-blocking syscalls. We can file new issues for specific problems.

Status changed to Fixed.

@rsc rsc added fixed labels Jul 15, 2013

@golang golang locked and limited conversation to collaborators Jun 24, 2016

This issue was closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.