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

Use bytes.Buffer instead channels and goroutines #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gaydin
Copy link

@gaydin gaydin commented May 31, 2019

Hello! :)
Thanks for the library
I made a few changes, namely: I got rid of goroutines and channels, which helped greatly increase performance

BenchmarkMakeQueryOld-4 30000 54336 ns/op 2537 B/op 29 allocs/op
BenchmarkMakeQueryBuffered-4 200000 6547 ns/op 744 B/op 15 allocs/op

BenchmarkMakeQueryOld-4        30000    54336 ns/op   2537 B/op   29 allocs/op
BenchmarkMakeQueryBuffered-4   200000   6547 ns/op    744 B/op    15 allocs/op
// Primitive Wrapper Types //
/////////////////////////////
// ///////////////////////////
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, go fmt

@CreatCodeBuild
Copy link
Contributor

CreatCodeBuild commented Jun 1, 2019

@gaydin It's nice to see people using it. It's great work to double the speed.

May you include your benchmark code as well so that the performance data is reproducible?

I'm the original creator of this lib. I don't work for Udacity anymore though. It's up to Udacity folks to decide if they want to merge it.

But I am curious how did you find this lib and what's your use cases.

"fmt"
)

type argumentValue interface {
stringChan() <-chan string
stringChan(buffer *bytes.Buffer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fantastic. I wanted to do this when I first created it. But I was too happy about the clever use of channels & goroutine so that I left it there. But I do think an io.Writer alike version is better.

Some suggestions:

  1. How about stringWriter() *bytes.Buffer which returns an writer instead of consuming one?
  2. Have you think about io.Writer?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function name could be stringWriter so that people don't get confused.

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.

2 participants