code review 8478044: time: add Time.FormatAppend

time: add Time.FormatAppend

This is a version of Time.Format that doesn't require allocation.

Fixes Issue 5192
Update Issue 5195

[bradfitz, 2013-05-14 14:55:00]

Hello golang-dev@googlegroups.com,

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

[gobot, 2013-05-14 22:14:42]

R=r (assigned by bradfitz)

[bradfitz, 2013-05-15 00:28:45]

Reuploaded, since Rietveld was showing its fun corrupt patch message.

Diff is tiny, but it adds API.

[r, 2013-05-15 21:51:44]

https://codereview.appspot.com/8478044/diff/11001/src/pkg/time/format.go
File src/pkg/time/format.go (right):

https://codereview.appspot.com/8478044/diff/11001/src/pkg/time/format.go#newc...
src/pkg/time/format.go:387: b = buf[:0]
move the declaration of buf to here.

but why even bother? why not just always make a buffer?

[bradfitz, 2013-05-15 22:03:14]

https://codereview.appspot.com/8478044/diff/11001/src/pkg/time/format.go
File src/pkg/time/format.go (right):

https://codereview.appspot.com/8478044/diff/11001/src/pkg/time/format.go#newc...
src/pkg/time/format.go:387: b = buf[:0]
On 2013/05/15 21:51:44, r wrote:
> move the declaration of buf to here.
> 
> but why even bother? why not just always make a buffer? 

This is just moving the code.  I didn't write this.

But this does save an allocation.  Only the [64]byte -> string copy is made,
rather than allocating a []byte and then allocating a string.

[r, 2013-05-15 22:05:05]

https://codereview.appspot.com/8478044/diff/11001/src/pkg/time/format.go
File src/pkg/time/format.go (right):

https://codereview.appspot.com/8478044/diff/11001/src/pkg/time/format.go#newc...
src/pkg/time/format.go:387: b = buf[:0]
fair enough. it shouldn't allocate even if you call make, but it does. i've
filed an issue.

i still think the allocation of buf should move inside the if.

[bradfitz, 2013-05-15 22:18:41]

PTAL

https://codereview.appspot.com/8478044/diff/11001/src/pkg/time/format.go
File src/pkg/time/format.go (right):

https://codereview.appspot.com/8478044/diff/11001/src/pkg/time/format.go#newc...
src/pkg/time/format.go:387: b = buf[:0]
On 2013/05/15 22:05:05, r wrote:
> fair enough. it shouldn't allocate even if you call make, but it does. i've
> filed an issue.
> 
> i still think the allocation of buf should move inside the if.

That means I have to add a const, or do:

if buf := [64]byte{}; max <= len(buf) {
   b = buf[:0]
}

I've went with a const.

[r, 2013-05-16 00:04:00]

LGTM

[bradfitz, 2013-05-16 00:24:28]

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

time: add Time.FormatAppend

This is a version of Time.Format that doesn't require allocation.

Fixes Issue 5192
Update Issue 5195

R=r
CC=gobot, golang-dev
https://codereview.appspot.com/8478044

[rsc, 2013-05-17 17:59:00]

This function breaks the usual Go convention of putting the destination as the
first argument (copy, io.Copy, strconv.AppendFloat, etc).

It also breaks the convention set by strconv of putting Append first, but I
think that's less important.

[r, 2013-05-17 18:53:23]

Let's rethink this. Can we roll it back?

[bradfitz, 2013-05-17 19:03:13]

Just decide what to name it and the argument order and I'll send a CL to
change it.

Why roll it back?
On May 17, 2013 11:53 AM, <r@golang.org> wrote:

> Let's rethink this. Can we roll it back?
>
>
>
https://codereview.appspot.**com/8478044/<https://codereview.appspot.com/8478...
>

[r, 2013-05-17 19:07:11]

It's ugly.

-rob

[bradfitz, 2013-05-17 19:14:14]

Constructive.
 On May 17, 2013 12:07 PM, "Rob Pike" <r@golang.org> wrote:

> It's ugly.
>
> -rob
>

[gustavo_niemeyer.net, 2013-05-17 19:18:55]

On Fri, May 17, 2013 at 4:14 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Constructive.

Just rollback and propose AppendFormat(dest, format)?  Seems easier
than debating.


gustavo @ http://niemeyer.net

[rsc, 2013-05-17 19:50:16]

It's a performance hack leaking into the API.

Russ

[gustavo_niemeyer.net, 2013-05-17 20:06:19]

On Fri, May 17, 2013 at 4:50 PM, Russ Cox <rsc@golang.org> wrote:
> It's a performance hack leaking into the API.

Isn't that the case all over the place? Why would we have
strconv.Append*, or the bytes vs. string duplication all over the
regexp package, or the special case of append(bytes, string...)?

It's a genuine question, rather than a trolling one. I don't quite get
where the line is.


gustavo @ http://niemeyer.net

[bradfitz, 2013-05-17 20:10:46]

Yeah, I have no clue what the rules are either.



On Fri, May 17, 2013 at 1:05 PM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> On Fri, May 17, 2013 at 4:50 PM, Russ Cox <rsc@golang.org> wrote:
> > It's a performance hack leaking into the API.
>
> Isn't that the case all over the place? Why would we have
> strconv.Append*, or the bytes vs. string duplication all over the
> regexp package, or the special case of append(bytes, string...)?
>
> It's a genuine question, rather than a trolling one. I don't quite get
> where the line is.
>
>
> gustavo @ http://niemeyer.net
>

[rsc, 2013-05-17 20:11:47]

On Fri, May 17, 2013 at 4:05 PM, Gustavo Niemeyer <gustavo@niemeyer.net>wrote:

> On Fri, May 17, 2013 at 4:50 PM, Russ Cox <rsc@golang.org> wrote:
> > It's a performance hack leaking into the API.
>
> Isn't that the case all over the place? Why would we have
> strconv.Append*, or the bytes vs. string duplication all over the
> regexp package, or the special case of append(bytes, string...)?
>
> It's a genuine question, rather than a trolling one. I don't quite get
> where the line is.
>

reasonable people can disagree about where the line is.
AppendFormat is certainly somewhere near it.

russ