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

OPTIMIZE: Change ServerBaseContext #137

Merged
merged 1 commit into from Jan 16, 2017
Merged

OPTIMIZE: Change ServerBaseContext #137

merged 1 commit into from Jan 16, 2017

Conversation

cyx
Copy link
Contributor

@cyx cyx commented Jan 16, 2017

Upon transitioning our production services to ServerBaseContext, we saw a serious regression with performance. It wasn't obvious, but I was able to narrow it down to a mysterious solution. Here's a gist of a reproducible benchmark:

https://gist.github.com/cyx/8730ab8d31996711110d9979d1715f98

Long story short, it seems if we create a copy of the base context, then we don't see this performance regression.

I'm open to discuss this some more -- and I've already picked the brains of other Go programmers in our company to see what they think. Logic dictates the copy doesn't do anything, but empirical data shows it does.

Thought this will be useful for others.

@pkieltyka
Copy link
Member

This is very helpful thank you @cyx cc: check this out @VojtechVitek

@pkieltyka pkieltyka merged commit 8dd9731 into go-chi:master Jan 16, 2017
@VojtechVitek
Copy link
Contributor

VojtechVitek commented Jan 16, 2017

Very nice catch! LGTM

Seems like the lines 118 and 121 store more and more data into baseCtx on each run, since it was a local variable in the parent function scope.

@VojtechVitek
Copy link
Contributor

VojtechVitek commented Jan 16, 2017

btw: Same pattern is used in the Throttler middleware (see https://github.com/pressly/chi/blob/master/middleware/throttler.go#L36)

I just wrote a simple use case, explaining the behavior, if you're interested: https://play.golang.org/p/wQXiuUgUKI

@cyx
Copy link
Contributor Author

cyx commented Jan 16, 2017

Perfect thanks for the quick turnaround. I'll get this pulled into our latest release and use it in our prod servers this week.

@cyx cyx deleted the patch-1 branch January 17, 2017 22:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants