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

time: optimize Parse and Time.Format for RFC3339 #54093

Closed
dsnet opened this issue Jul 27, 2022 · 8 comments
Closed

time: optimize Parse and Time.Format for RFC3339 #54093

dsnet opened this issue Jul 27, 2022 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Jul 27, 2022

RFC3339 is the most common format to use (according to prior analysis, it accounts for 57% of formats; and that's only explicit calls to Format or Parse. It doesn't cover all the implicit cases that occur through MarshalText and UnmarshalText). We should optimize Parse and Time.Format to do a quick check for the RFC3339Nano or RFC3339 constants and hardcode the logic for that format. This would avoid the double parsing that currently occurs where we parse the format string to functionally run a virtual machine that then parses the input value.

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 27, 2022
@cherrymui cherrymui added this to the Backlog milestone Jul 27, 2022
@cherrymui
Copy link
Member

cc @ianlancetaylor @rsc

@amarjeetanandsingh
Copy link
Contributor

amarjeetanandsingh commented Aug 3, 2022

Is it appropriate to offer my contribution here?

@cherrymui
Copy link
Member

Sure, your contribution is welcomed. See https://go.dev/doc/contribute for how to contribute to Go. Thanks.

@dsnet
Copy link
Member Author

dsnet commented Aug 3, 2022

Happy to review.

@amarjeetanandsingh
Copy link
Contributor

Sure.
thanks @dsnet @cherrymui

I’ll be back shortly with initial implementation.

@gopherbot
Copy link

Change https://go.dev/cl/421877 mentions this issue: time: optimize Format and Parse for RFC3339 and RFC3339Nano

@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Aug 9, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Aug 9, 2022
@gopherbot
Copy link

Change https://go.dev/cl/425100 mentions this issue: time: add fuzz test for Time.appendFormatRFC3339

@gopherbot
Copy link

Change https://go.dev/cl/425197 mentions this issue: time: optimize Parse for RFC3339 and RFC3339Nano

gopherbot pushed a commit that referenced this issue Aug 29, 2022
Time.appendFormatRFC3339 is a specialized implementation of
Time.appendFormat. We expect the two to be identical.
Add a fuzz test to ensure this property.

Updates #54093

Change-Id: I0bc41ee6e016d3dac75d1ac372d8c9c7266d0299
Reviewed-on: https://go-review.googlesource.com/c/go/+/425100
Reviewed-by: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
gopherbot pushed a commit that referenced this issue Sep 13, 2022
RFC 3339 is the most common time representation,
being used in an overwhelming 57.3% of all specified formats,
while the next competitor only holds 7.5% usage.
Specially optimize parsing to handle the RFC 3339 format.
To reduce the complexity of error checking,
parseRFC3339 simply returns a bool indicating parsing success.
It leaves error handling to the general parse path.

To assist in fuzzing, the internal parse function was left unmodified
so that we could test that parseRFC3339 and parse agree with each other.

Performance:

	name             old time/op  new time/op  delta
	ParseRFC3339UTC  112ns ± 1%   37ns ± 1%    -67.37%  (p=0.000 n=9+9)
	ParseRFC3339TZ   259ns ± 2%   67ns ± 1%    -73.92%  (p=0.000 n=10+9)

Credit goes to Amarjeet Anand for a prior CL attemping to optimize this.
See CL 425014.

Fixes #54093

Change-Id: I14f4e8c52b092d44ceef6863f261842ed7e83f4c
Reviewed-on: https://go-review.googlesource.com/c/go/+/425197
Reviewed-by: Rob Pike <r@golang.org>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Jenny Rakoczy <jenny@golang.org>
@golang golang locked and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

5 participants