use AppendFormat to format time values

[achille-roussel]

Description

This pull request changes the method used to format time values from Format to AppendFormat. The intent is to prevent the dynamic memory allocation that occurs when generating the returned string.

Checklist

• Code compiles correctly
• Created tests which fail without the change (if possible)
• All tests passing
• Extended the README / documentation, if necessary
• Added myself / the copyright holder to the AUTHORS file

[julienschmidt]

Have you checked if there actually is a dynamic allocation or if the compiler is smart enough to optimize it away in this case?

The tests are currently failing, since AppendFormat is available only in Go 1.5+, however dropping support for old versions which are not officially supported by the Go team anymore anyway, shouldn't be a problem.

[achille-roussel]

Yes I saw it surface in code I was optimizing, the compiler doesn't know how to optimize the return value, which is why the Append* functions and methods exist.

Here's a screenshot from a memory allocation profile I was digging in (the original code is not open source unfortunately):

Which went away then after changing it to use AppendFormat.

I'm gonna make this a conditional change that only gets compiled on Go 1.5+

Thanks for the feedback!

[methane]

No need to keep Go 1.4 support.
Very old Go versions are tested because just there were no reason to remove them.
For security concern, no one should use even Go 1.5, at least when writing new code.

[methane]

https://golang.org/doc/devel/release.html#policy

As a special case, we issue minor revisions for critical security problems in both the current release and the previous release. For example, if Go 1.5 is the current release then we will issue minor revisions to fix critical security problems in both Go 1.4 and Go 1.5 as they arise.

Current Go is 1.8 and Go 1.7 will get critical security fix.
But since GAE/Go is still use Go 1.6, I think Go 1.6 is "used" version.
But I think no one should use Go 1.5 or earlier, at least for new code.

[julienschmidt]

We just dropped support for Go 1.3 and earlier with 72e0ac3. Also removing Go 1.4 would be completely fine IMO.
I suggest that our guideline should be to support at least the last 3 versions + tip.

[julienschmidt]

Actually it would be beneficial to only support Go 1.5+ as we then could start to reorganize our code into internal sub-packages.
In any case, we definitely don't need 2 more files containing just 1 line of code just to keep support for an already long unsupported Go version.

[achille-roussel]

OK, I'll revert the last two commits and edit the travis file to not run on Go 1.4 and below, does that sound good?

[julienschmidt]

Preferably rebase on the current master head and also edit the README: 72e0ac3#diff-04c6e90faac2675aa89e2176d2eec7d8L42

[achille-roussel]

Cool 👍

[achille-roussel]

This should be good to go, let me know if anything else should be changed.

[achille-roussel]

Thanks for the quick review!

[julienschmidt]

Thanks for contributing!
Let us know if you find any other possible optimizations

[rkuska]

Sorry for commenting on old piece of code but I came here from an article and I can't help but wonder, why this wasn't done by make([]byte, 0, 64). Isn't it the same?

[achille-roussel]

I think the compiler can optimize “make” to be stack allocated if possible, so the end result is the same whether we use make or declare an array, it’s just a style decision.

The optimization doesn’t come from the use of an array, it comes from writing directly into a pre-allocated buffer instead of working with intermediary heap allocated values.