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 AppendFormat to format time values #615

Merged
merged 4 commits into from Jun 10, 2017

Conversation

Projects
None yet
3 participants
@achille-roussel
Contributor

achille-roussel commented Jun 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@julienschmidt

julienschmidt Jun 10, 2017

Member

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.

Member

julienschmidt commented Jun 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@achille-roussel

achille-roussel Jun 10, 2017

Contributor

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):
screen shot 2017-06-10 at 10 32 49 am
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!

Contributor

achille-roussel commented Jun 10, 2017

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):
screen shot 2017-06-10 at 10 32 49 am
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

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jun 10, 2017

Contributor

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.

Contributor

methane commented Jun 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@methane

methane Jun 10, 2017

Contributor

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.

Contributor

methane commented Jun 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@julienschmidt

julienschmidt Jun 10, 2017

Member

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.

Member

julienschmidt commented Jun 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@julienschmidt

julienschmidt Jun 10, 2017

Member

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.

Member

julienschmidt commented Jun 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@achille-roussel

achille-roussel Jun 10, 2017

Contributor

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?

Contributor

achille-roussel commented Jun 10, 2017

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

This comment has been minimized.

Show comment
Hide comment
@julienschmidt

julienschmidt Jun 10, 2017

Member

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

Member

julienschmidt commented Jun 10, 2017

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

@achille-roussel

This comment has been minimized.

Show comment
Hide comment
@achille-roussel

achille-roussel Jun 10, 2017

Contributor

Cool 👍

Contributor

achille-roussel commented Jun 10, 2017

Cool 👍

@achille-roussel

This comment has been minimized.

Show comment
Hide comment
@achille-roussel

achille-roussel Jun 10, 2017

Contributor

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

Contributor

achille-roussel commented Jun 10, 2017

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

@julienschmidt julienschmidt merged commit a825be0 into go-sql-driver:master Jun 10, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 75.462%
Details
@achille-roussel

This comment has been minimized.

Show comment
Hide comment
@achille-roussel

achille-roussel Jun 10, 2017

Contributor

Thanks for the quick review!

Contributor

achille-roussel commented Jun 10, 2017

Thanks for the quick review!

@achille-roussel achille-roussel deleted the achille-roussel:optimize-time-format branch Jun 10, 2017

@julienschmidt

This comment has been minimized.

Show comment
Hide comment
@julienschmidt

julienschmidt Jun 10, 2017

Member

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

Member

julienschmidt commented Jun 10, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment