Skip to content

Use strings concat where possible in the request signing #729

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

Closed
wants to merge 1 commit into from

Conversation

dlsniper
Copy link

Hi,

This is a small improvement, but one nonetheless.
On my machine, this: https://play.golang.org/p/a_ow6e7II7 using go 1.7 beta2 produces the following results:

go test -bench=.
BenchmarkStringsJoin-8          10000000               140 ns/op
BenchmarkStringsConcat-8        20000000                92.6 ns/op
PASS
ok      some/package        3.505s


go test -bench=. -benchmem
BenchmarkStringsJoin-8          10000000               136 ns/op             128 B/op          2 allocs/op
BenchmarkStringsConcat-8        20000000                96.6 ns/op            64 B/op          1 allocs/op
PASS
ok      some/package        3.565s

Also I wonder if it isn't a mistake that we have two new lines before v4.canonicalHeaders and not after?

Thank you.

@jasdel
Copy link
Contributor

jasdel commented Jun 23, 2016

Thanks for taking the time to submit this PR @dlsniper. I don't think this will be a great change to take. The performance difference in negligible, and arguable hurts readability.

Right now these sections are a little gnarly to read. I'm not sure if a fmt.Sprintf would make them any easier to read.

@jasdel jasdel added feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. closing-soon This issue will automatically close in 4 days unless further comments are made. and removed feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jun 27, 2016
@dlsniper
Copy link
Author

Hi, sorry for the delayed reply. For many of the usecases yes, this might make no difference. I do think that for high perf environments, even a few ns shaved of while generating the key would be nice but up to you. I can rebase if you want or close this. Please let me know which direction you'd prefer. Thank you.

@jasdel
Copy link
Contributor

jasdel commented Jun 30, 2016

@dlsniper, thanks for the update. I think the SDK would be better served using pprof/profiling to identify big fish within the SDK that can make significant improvements. Once we've knocked out those items I think that then is a good time to consider micro optimizations vs the cost of readability and maintainability.

One item I'm working on right now is to collect metrics for performance analysis of the SDK so we can identify areas to target for further improvements. Please continue to investigate and bring up areas you find where the SDK's performance could be improved.

@jasdel jasdel closed this Jun 30, 2016
@jasdel jasdel removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jun 30, 2016
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