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

speed up input #82

Closed
wants to merge 2 commits into from
Closed

speed up input #82

wants to merge 2 commits into from

Conversation

tzneal
Copy link
Contributor

@tzneal tzneal commented Sep 22, 2016

There are two changes, one to enable some benchmarking and a second that speeds up counting glyphs.

I kept them separate as you may just want the second change without all of the kludging to allow tests to trick the library into thinking it's talking to a terminal instead of a pipe.

Allow specifying a reader, writer and file descriptors to enable
benchmarking
Speed up glyph counting by handling the common case first.  With this
change, most of the time is now spent redrawing the line (Fprint)
instead of counting characters.

benchmark                old ns/op     new ns/op     delta
BenchmarkInput1-4        15361         13886         -9.60%
BenchmarkInput10-4       121755        99348         -18.40%
BenchmarkInput100-4      2229504       1022654       -54.13%
BenchmarkInput1000-4     168239689     15721954      -90.66%

Updates #81
@tzneal
Copy link
Contributor Author

tzneal commented Sep 24, 2016

I'd also appreciate your thoughts about making the function visible, something like:

func NewLinerWith(r io.Reader, w io.Writer, inFD int, outFD int) *State

to allow end to end unit tests by dependent libraries.

@peterh
Copy link
Owner

peterh commented Nov 3, 2016

Sorry for the late reply.

I tried to pull, but it broke go test on Windows. If you'd rather not fix the benchmark on Windows, I can cherry-pick the "speed up input" commit. We can keep the benchmark in a separate branch.

Re: NewLinerWith. The main problem with NewLinerWith is the Windows input layer uses console handles instead of file descriptors, so even if you implement it on Windows, the function signature could change between uintptr and int. I don't mind if unexported functions have different signatures on different platforms, but I'd really rather not export a function that does not have a stable signature.

@tzneal
Copy link
Contributor Author

tzneal commented Nov 3, 2016

No problem, I haven't had much of a chance to work on my personal project as of late anyway.

Go ahead and cherry pick the speed up commit, I wasn't happy with what I had to do to get the benchmark working anyway. I doubt there's much left in the way of low hanging fruit on the optimization front as well.

@peterh
Copy link
Owner

peterh commented Nov 3, 2016

Okay, I've cherry-picked the speed-up commit into master, and cherry-picked the benchmark rework commit into bench (in case I need to reference it again).

Thanks! The benchmark rework was especially helpful when I was making more speed improvements.

@peterh peterh closed this Nov 3, 2016
@tzneal
Copy link
Contributor Author

tzneal commented Nov 3, 2016

Great, glad to hear it!

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

2 participants