code review 7315043: net/http: Format HTTP-date string every second.

net/http: Format HTTP-date string every second.
(See https://gist.github.com/methane/4715414 for benchmark)

[songofacandy, 2013-02-06 01:31:49]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[mattn, 2013-02-06 01:39:01]

LGTM

https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:39: var httpDate string
I prefer to add more comment that is updated in updateHttpDate() each seconds.

https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1337: stop := updateHttpDate()
As you know, this part should be singleton. :)

[dave_cheney.net, 2013-02-06 01:43:36]

NOT LGTM. This introduces a data race. 

Please discussion your proposal on the mailing list first.

[mattn, 2013-02-06 01:51:16]

> NOT LGTM. This introduces a data race. 
> 
> Please discussion your proposal on the mailing list first.

I thinkg it can solve with creating groutine at once by sync.Once.

[minux1, 2013-02-06 01:56:11]

https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:39: var httpDate string
this is racy.

[mattn, 2013-02-06 02:06:53]

https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:39: var httpDate string
On 2013/02/06 01:56:11, minux wrote:
> this is racy.

minus, how about this? I guess, below is not racy.

package main

import (
	"fmt"
	"time"
)

var httpDate *string
const TimeFormat = "Mon, 02 Jan 2006 15:04:05 GMT"

func updateHttpDate() chan<- bool {
	if httpDate != nil {
		return nil
	}
	date := ""
	httpDate = &date
	date = time.Now().UTC().Format(TimeFormat)
	stop := make(chan bool)
	go func() {
		tick := time.Tick(time.Second)
		for {
			select {
			case <-stop:
				return
			case t := <-tick:
				date = t.UTC().Format(TimeFormat)
			}
		}
	}()
	return stop
}

func main() {
	foo := updateHttpDate()
	bar := updateHttpDate() // bar shold be nil

	fmt.Println(*httpDate)

	if foo != nil {
		println("stop foo channel")
	}
	if bar != nil {
		println("stop bar channel") // not passed.
	}
}

[songofacandy, 2013-02-06 02:09:27]

I see. I posted updated patch to
https://groups.google.com/forum/?fromgroups=#!topic/golang-nuts/zeLMYnjO_JA

On 2013/02/06 01:43:36, dfc wrote:
> NOT LGTM. This introduces a data race. 
> 
> Please discussion your proposal on the mailing list first.

[songofacandy, 2013-02-06 02:34:45]

My post has not shown...
I've posted this gist: https://gist.github.com/methane/4719273

[songofacandy, 2013-02-06 02:57:03]

Hello golang-dev@googlegroups.com, mattn.jp@gmail.com, dave@cheney.net,
minux.ma@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[songofacandy, 2013-02-06 03:47:47]

I understand that string is not an atomic type.
I'll update patch with RWMutex.

[songofacandy, 2013-02-06 03:52:25]

Hello golang-dev@googlegroups.com, mattn.jp@gmail.com, dave@cheney.net,
minux.ma@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[minux1, 2013-02-06 14:05:02]

https://codereview.appspot.com/7315043/diff/6002/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/7315043/diff/6002/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:514: go func() {
this is still racy and can create more than one updating
goroutines and introduce more unnecessary contention
on the rwmutex.

there are several ways to fix the data race, but we probably
should discuss the design more.

[disposaboy_dby.me, 2013-02-08 16:17:12]

On Wednesday, February 6, 2013 1:39:01 AM UTC, mattn wrote:
>
> LGTM 
>
>
> https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go 
> File src/pkg/net/http/server.go (right): 
>
>
https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go#n...

>
>
src/pkg/net/http/server.go:39<https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go#newcode39src/pkg/net/http/server.go:39>:

> var httpDate string 
> I prefer to add more comment that is updated in updateHttpDate() each 
> seconds. 
>
>
https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go#n...

>
>
src/pkg/net/http/server.go:1337<https://codereview.appspot.com/7315043/diff/1002/src/pkg/net/http/server.go#newcode1337src/pkg/net/http/server.go:1337>:

> stop := updateHttpDate() 
> As you know, this part should be singleton. :) 
>
> https://codereview.appspot.com/7315043/ 
>


It's possible that I may mis-understand the memory model and what's going 
on here but assuming my assumptions are correct:

* the purpose of this code is to essentially cache the http format which 
doesn't from second to second
* the memory mode permits safe swapping of pointers (the string hDate 
points to is never updated so I don't believe there can be any data-race)

I believe http://play.golang.org/p/2aN1bSQ0Ld achieves this requirement 
without need of locking - it doesn't include the option to stop the 
goroutine but I'm not sure what the need for that is and AFAICS it'd cause 
issues if it was ever stopped

 

[rsc, 2013-02-08 16:27:27]

NOT LGTM

What is the purpose of this CL? Is it to beat some other system on a
microbenchmark? It is difficult for me to believe that this matters in a
real server.

[songofacandy, 2013-02-08 17:40:13]

On 2013/02/08 16:27:27, rsc wrote:
> NOT LGTM
> 
> What is the purpose of this CL? Is it to beat some other system on a
> microbenchmark? It is difficult for me to believe that this matters in a
> real server.

HTTP server is not only for serving Web pages.
I want to use Go for building proxy server, load balancer and API servers.
These real servers may require 1k~10k+ request/sec performance.

Additionally, this CL reveals effect of low level optimization.
I hope future version of Go is fast like HTTP servers written in C.

[rsc, 2013-02-08 20:41:22]

Before we go any further Brad needs to argue that this should go forward.
As I said, I doubt this matters.

If we were going to do this, this is how to do it:

var date struct {
    sync.RWMutex
    t time.Time
    s string
}

func dateString() string {
    now := time.Now().Round(1*time.Second)
    date.RLock()
    t := date.t
    s := date.s
    date.RUnlock()
    if !now.After(t) {
        return s
    }
    date.Lock()
    if now.After(date.t) {
        date.t = now
        date.s = now.Format(TimeFormat)
    }
    s := date.s
    date.Unlock()
    return s
}

I think this will cause contention in active servers that may well negate
any savings.

Russ

[bradfitz, 2013-02-08 21:05:49]

I used to care more, before time.Format got faster. (Hence my old Format CL)

There's still a bug open to make it faster, IIRC.

I haven't looked at dl.google.com profiles in a few months. It used to show
up, but not sure lately. I can look.
On Feb 8, 2013 12:41 PM, "Russ Cox" <rsc@golang.org> wrote:

> Before we go any further Brad needs to argue that this should go forward.
> As I said, I doubt this matters.
>
> If we were going to do this, this is how to do it:
>
> var date struct {
>     sync.RWMutex
>     t time.Time
>     s string
> }
>
> func dateString() string {
>     now := time.Now().Round(1*time.Second)
>     date.RLock()
>     t := date.t
>     s := date.s
>     date.RUnlock()
>     if !now.After(t) {
>         return s
>     }
>     date.Lock()
>     if now.After(date.t) {
>         date.t = now
>         date.s = now.Format(TimeFormat)
>     }
>     s := date.s
>     date.Unlock()
>     return s
> }
>
> I think this will cause contention in active servers that may well negate
> any savings.
>
> Russ
>

[bradfitz, 2013-06-04 21:30:19]

R=close

(testing something)