Important performance improvement about gin HOT 19 CLOSED

@manucorporat
I run the example and see the following error.

Done! in path /long_async
panic: http: wrote more than the declared Content-Length

from gin.

We also can extend this new system to HttpRouter, so we could get 0 allocs for all operations!

from gin.

That's amazing! 21'89% faster!

from gin.

Check it out:
10979dd

from gin.

Great performance!
But commit 10979dd seems to have a little danger logic.
If HandlerFunc code has goroutine which have a heavy task, the same gin.Context has a risk of overwritten by other requests.
Examples:

r.GET("/goroutine", func(c *gin.Context) {
  go func() {
    // some heavy task
    for i:= 0; i < 10000000000; i++ { _ = i * i }

    // This header is a possible other request’s.
    fmt.Println(c.Req.Header)
  }
  c.String(200, "ok")
}     

This problem is caused by re-using gin.Context is still processing on goroutine.
Easy to confirmed this problem If gin.Engine cacheSize reduce to minimum.

from gin.

can you explain how could that happen? Maybe there are a bug, but the actual implementation should do this:

Using 'select' get a free pointer from the cache channel, once a pointer is returned from a channel (it's like a queue), that pointer is not longer there. Using the same select, if the cache can't return any free pointer, a new Context is allocated (see the default case).

When the request finish, reuseContext() is called. It also uses a select to prevent blocking in case the cache channel is full (see the default case).

If cacheSize is small and your requests are heavy (can be tested with time.Sleep(5*time.Second)), the system would behaviour like it has no cache at all.

Note this: "In Go, a buffered channel is just that: a thread-safe FIFO queue"

from gin.

There is indeed a bug when using long running go routines. It shows in this[0] example. To reproduce:

• Start the server.
• Hit /long one itme
• Hit the start page (/) 5 times.
• Wait..
• Watch Url (only available via /long): / get printed.

However, i'm not sure that long-running go routines should be supported in the standard GET/POST/etc handlers so this might be a non-issue. Maybe we could have a special solution for async routes?

I also noticed that CacheStress is unused.

[0] https://gist.github.com/alexandernyquist/5d67ac18b6fd344cf183

from gin.

I understand, I did not notice the example was using a goroutine.
Two ideas:

1. Remove caching support. Adding features to context will make the whole system slower.
2. Add a NotReuse() method.

from gin.

r.GET("/goroutine", func(c *gin.Context) {
  c.NotReuse() /// <<<<<
  // or
  c.Reuse = false
  go func() {
    // some heavy task
    for i:= 0; i < 10000000000; i++ { _ = i * i }

    // This header is a possible other request’s.
    fmt.Println(c.Req.Header)
  }
  c.String(200, "ok")
}

I think the 90% of the times, nobody uses a goroutine inside a http handler. And you only need to call NotReuse() if you are going to use the Context information inside the new goroutine.

Anyway the ResponseWriter doesn't work either here: https://gist.github.com/manucorporat/10d4dd62d8992ed33479

from gin.

I agree that it's a corner case.

Adding a NotReuse (or maybe some other name, with a non-negative word) seems like a good solution. If we go this way, there should also be a way to signal that the context can be reused.

Maybe something like this?

r.GET("/goroutine", func(c *gin.Context) {
  c.Keep() // Do not reuse immediately

  go func() {
    // some heavy task
    for i:= 0; i < 10000000000; i++ { _ = i * i }

    // This header is a possible other request’s.
    fmt.Println(c.Req.Header)

    c.Release()
  }
  c.String(200, "ok")
}

from gin.

I was about to suggest the same thing! let's do it
Keep(), Release()
or
Retain(), Release()?

Simple keep-> release, or retain count? (my objective-c past is coming... haha)

from gin.

Retain(), Release() could be good if you have several goroutines... but then we have to protect them with a lock...

from gin.

d9573b4

from gin.

I wonder if we really need to deal with this corner case right now.

An even simpler solution could be to have a Context.Copy()-method which you use inside the goroutine. This context is never cached and collected when the goroutine finishes. Like:

r.GET("/goroutine", func(c *gin.Context) {
  cc := c.Copy()/Clone()
  go func() {
    // some heavy task
    for i:= 0; i < 10000000000; i++ { _ = i * i }

    // This header is a possible other request’s.
    fmt.Println(cc.Req.Header)
  }
  c.String(200, "ok")
}

from gin.

In my opinion, Copy() could be more bug prone, since you should perform a deep copy. And slow.
At the end of the day: Copy() is the same than NotReuse()

from gin.

It's a tricky API. If we use Keep() and Release(), the fact that you miss a call to Releade() is not a bug or memory leak at all, it simply will not get reused.

from gin.

In my case, there is a use-case for passing the context around even after the request has returned (to longer-running goroutines kicked-off by the request). I suppose I can copy items out of the context, but there is convenience in having it hang around.

While I love the speed of things, a few nanoseconds at the cost of convenience and safety (and more importantly, isolation) between requests seems like an over-optimization?

Copy seems like a solution that at least gives me the ability to create isolation, but a potential overwrite of the context in any situation - long or short run - is rather concerning to me.

from gin.

@themartorana you are right, we are going to use Copy()

• Easy to understand
• Safe
• Not slow at all
• Just 1 API, you do not have to Release()

from gin.

I added an example code in the README:
https://github.com/gin-gonic/gin/tree/develop#goroutines-inside-a-middleware

from gin.