code review 8179043: net/textproto: reduce allocations in ReadMIMEHeader

net/textproto: reduce allocations in ReadMIMEHeader

ReadMIMEHeader is used by net/http, net/mail, and
mime/multipart.

Don't do so many small allocations. Calculate up front
how much we'll probably need.

benchmark                  old ns/op    new ns/op    delta
BenchmarkReadMIMEHeader         8433         7467  -11.45%

benchmark                 old allocs   new allocs    delta
BenchmarkReadMIMEHeader           23           14  -39.13%

benchmark                  old bytes    new bytes    delta
BenchmarkReadMIMEHeader         1705         1343  -21.23%

[bradfitz, 2013-03-29 21:58:38]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[r, 2013-03-29 22:18:04]

this sets a precedent i'd rather leave unset.

https://codereview.appspot.com/8179043/diff/4001/src/pkg/net/textproto/reader.go
File src/pkg/net/textproto/reader.go (right):

https://codereview.appspot.com/8179043/diff/4001/src/pkg/net/textproto/reader...
src/pkg/net/textproto/reader.go:14: "unsafe"
if you do this it's a problem on app engine, isn't it? i'd much prefer not to
use unsafe for speed like this.  if you absolutely insist, put the unsafe piece
in a separate build-tagged file.

[bradfitz, 2013-03-29 22:25:19]

App Engine trusts the standard library.

Do you still want it in a separate file?

I'm of the mindset that,
-- Go is a language about giving you control over memory.
-- The standard library is allowed to be fast, so others can use it and
assume it's fast and not have to play their own games.
-- I'd rather we play games than everybody else play games (or, say, have
the net/http package provide a hook for users to supply their own alternate
reader functions. See also: the rejected CL for crypto sha1/rc4 alternate
implementation hooks)
-- If we don't do stuff like this, we don't see the pain points of the
language (e.g. Issue 1642) and we'll forget to fix it later.

This wouldn't even be objectionable if the language let you control the
capacity of a slice (Issue 1642), of which only the syntax is really
contentious.

I can

a) abandon this
b) move unsafe bits to a separate file (tagged what? appengine? unnecessary)
c) submit as-is.

?

On Fri, Mar 29, 2013 at 3:18 PM, <r@golang.org> wrote:

> this sets a precedent i'd rather leave unset.
>
>
> https://codereview.appspot.**com/8179043/diff/4001/src/pkg/**
>
net/textproto/reader.go<https://codereview.appspot.com/8179043/diff/4001/src/pkg/net/textproto/reader.go>
> File src/pkg/net/textproto/reader.**go (right):
>
> https://codereview.appspot.**com/8179043/diff/4001/src/pkg/**
>
net/textproto/reader.go#**newcode14<https://codereview.appspot.com/8179043/diff/4001/src/pkg/net/textproto/reader.go#newcode14>
> src/pkg/net/textproto/reader.**go:14: "unsafe"
> if you do this it's a problem on app engine, isn't it? i'd much prefer
> not to use unsafe for speed like this.  if you absolutely insist, put
> the unsafe piece in a separate build-tagged file.
>
>
https://codereview.appspot.**com/8179043/<https://codereview.appspot.com/8179...
>

[bradfitz, 2013-03-29 22:42:47]

Stand by.

Moving it to a separate file.


On Fri, Mar 29, 2013 at 3:25 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> App Engine trusts the standard library.
>
> Do you still want it in a separate file?
>
> I'm of the mindset that,
> -- Go is a language about giving you control over memory.
> -- The standard library is allowed to be fast, so others can use it and
> assume it's fast and not have to play their own games.
> -- I'd rather we play games than everybody else play games (or, say, have
> the net/http package provide a hook for users to supply their own alternate
> reader functions. See also: the rejected CL for crypto sha1/rc4 alternate
> implementation hooks)
> -- If we don't do stuff like this, we don't see the pain points of the
> language (e.g. Issue 1642) and we'll forget to fix it later.
>
> This wouldn't even be objectionable if the language let you control the
> capacity of a slice (Issue 1642), of which only the syntax is really
> contentious.
>
> I can
>
> a) abandon this
> b) move unsafe bits to a separate file (tagged what? appengine?
> unnecessary)
> c) submit as-is.
>
> ?
>
> On Fri, Mar 29, 2013 at 3:18 PM, <r@golang.org> wrote:
>
>> this sets a precedent i'd rather leave unset.
>>
>>
>> https://codereview.appspot.**com/8179043/diff/4001/src/pkg/**
>>
net/textproto/reader.go<https://codereview.appspot.com/8179043/diff/4001/src/pkg/net/textproto/reader.go>
>> File src/pkg/net/textproto/reader.**go (right):
>>
>> https://codereview.appspot.**com/8179043/diff/4001/src/pkg/**
>>
net/textproto/reader.go#**newcode14<https://codereview.appspot.com/8179043/diff/4001/src/pkg/net/textproto/reader.go#newcode14>
>> src/pkg/net/textproto/reader.**go:14: "unsafe"
>> if you do this it's a problem on app engine, isn't it? i'd much prefer
>> not to use unsafe for speed like this.  if you absolutely insist, put
>> the unsafe piece in a separate build-tagged file.
>>
>>
https://codereview.appspot.**com/8179043/<https://codereview.appspot.com/8179...
>>
>
>

[bradfitz, 2013-03-29 22:54:28]

Hello golang-dev@googlegroups.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-03-29 22:55:35]

PTAL

Now it'll involve less paperwork.

On Fri, Mar 29, 2013 at 3:54 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, r@golang.org (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
https://codereview.appspot.**com/8179043/<https://codereview.appspot.com/8179...
>

[r, 2013-03-29 23:07:24]

I believe encoding/gob is the only standard package that uses unsafe to speed up
allocations, and I wish I hadn't stepped into that water. I do not sanction
doing this in the standard library.

Please abandon this CL or find a way to improve the allocation strategy without
putting details about the runtime's internal data structures into a MIME parser
(!).

[bradfitz, 2013-03-29 23:12:57]

Maybe I can use gob instead.

On Fri, Mar 29, 2013 at 4:07 PM, <r@golang.org> wrote:

> I believe encoding/gob is the only standard package that uses unsafe to
> speed up allocations, and I wish I hadn't stepped into that water. I do
> not sanction doing this in the standard library.
>
> Please abandon this CL or find a way to improve the allocation strategy
> without putting details about the runtime's internal data structures
> into a MIME parser (!).
>
>
https://codereview.appspot.**com/8179043/<https://codereview.appspot.com/8179...
>

[bradfitz, 2013-03-29 23:16:13]

*** Abandoned ***

[iant, 2013-03-29 23:21:27]

Let's not work around a language/runtime problem by importing unsafe (unless
we're desperate).

[bradfitz, 2013-03-29 23:31:32]

I moved my whining to Issue 1642.

I was hoping this CL would be more acceptable than my previous one in that
it added no new public API and we could just change the one unsafe function
to use slice = slice[off:len:cap] or something later, if that ever happens.

On Fri, Mar 29, 2013 at 4:21 PM, <iant@golang.org> wrote:

> Let's not work around a language/runtime problem by importing unsafe
> (unless we're desperate).
>
>
>
https://codereview.appspot.**com/8179043/<https://codereview.appspot.com/8179...
>

[bradfitz, 2013-07-02 13:21:19]

Hello golang-dev@googlegroups.com, r@golang.org, iant@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-07-02 13:24:23]

PTAL, 3 months later.

Now a small patch, using s[i:j:k] instead of unsafe.




On Tue, Jul 2, 2013 at 6:21 AM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, r@golang.org, iant@golang.org (cc:
> golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
https://codereview.appspot.**com/8179043/<https://codereview.appspot.com/8179...
>

[adg, 2013-07-03 04:15:12]

LGTM

[bradfitz, 2013-07-03 05:37:30]

*** Submitted as https://code.google.com/p/go/source/detail?r=bb98e4c9b6b3 ***

net/textproto: reduce allocations in ReadMIMEHeader

ReadMIMEHeader is used by net/http, net/mail, and
mime/multipart.

Don't do so many small allocations. Calculate up front
how much we'll probably need.

benchmark                  old ns/op    new ns/op    delta
BenchmarkReadMIMEHeader         8433         7467  -11.45%

benchmark                 old allocs   new allocs    delta
BenchmarkReadMIMEHeader           23           14  -39.13%

benchmark                  old bytes    new bytes    delta
BenchmarkReadMIMEHeader         1705         1343  -21.23%

R=golang-dev, r, iant, adg
CC=golang-dev
https://codereview.appspot.com/8179043

[rsc, 2013-07-08 19:09:54]

LGTM