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

DateTime parsing regression #50612

Closed
gbaraldi opened this issue Jul 20, 2023 · 6 comments · Fixed by #50670
Closed

DateTime parsing regression #50612

gbaraldi opened this issue Jul 20, 2023 · 6 comments · Fixed by #50670
Labels
regression Regression in behavior compared to a previous version
Milestone

Comments

@gbaraldi
Copy link
Member

#50367 Caused a regression in the run(BaseBenchmarks.SUITE[["dates", "parse", "DateTime"]]) test of BaseBenchmarks.jl @BioTurboNick

@gbaraldi gbaraldi added the regression Regression in behavior compared to a previous version label Jul 20, 2023
@BioTurboNick
Copy link
Contributor

Is it possible to figure out exactly what method(s) slowed down?

@BioTurboNick
Copy link
Contributor

1.9.2:

using BenchmarkTools, Dates
@btime map(string, range(DateTime("2016-02-19T12:34:56"), step = Dates.Millisecond(123), length = 200));
  126.700 μs (4437 allocations: 218.20 KiB)

#50367:

@btime map(string, range(DateTime("2016-02-19T12:34:56"), step = Dates.Millisecond(123), length = 200));
  103.700 μs (4437 allocations: 218.20 KiB)

I'm not seeing a regression? Or I didn't find the right one.

@gbaraldi
Copy link
Member Author

@BioTurboNick
Copy link
Contributor

Hmm... It's not super obvious what's going on here. Profiling some of the functions doesn't show anything obviously passing through reinterpret. Inspected a couple levels of @code_native but they're identical.

_datetime_str = "2016-02-19T12:34:56.78"
_date_str = "2016-02-19"

DateTime($_datetime_str) # 51.976 ns (0 allocations: 0 bytes) vs. 97.574 ns (0 allocations: 0 bytes)
Date($_date_str) # 38.004 ns (0 allocations: 0 bytes) vs. 57.841 ns (0 allocations: 0 bytes)

_custom_date_str = "20160219"
_custom_date_fmt = "yyyymmdd"
_custom_datetime_str = "20160219 123456.78"
_custom_datetime_fmt = "yyyymmdd HHMMSS.sss"

Date($_custom_date_str, $_custom_date_fmt) # 2.144 μs (21 allocations: 1.16 KiB) vs. 2.411 μs (21 allocations: 1.16 KiB)
DateTime($_custom_datetime_str, $_custom_datetime_fmt) # 4.157 μs (46 allocations: 2.72 KiB) vs. 4.629 μs (46 allocations: 2.72 KiB)

DateTime($_datetime_str, $(Dates.ISODateTimeFormat)) # 51.521 ns (0 allocations: 0 bytes)
Date($_date_str, $(Dates.ISODateFormat)) # 38.951 ns (0 allocations: 0 bytes) vs. 97.992 ns (0 allocations: 0 bytes)
DateTime("Sat, 12 Nov 2016 07:45:36", $(Dates.RFC1123Format)) # 168.477 ns (0 allocations: 0 bytes) vs. 206.903 ns (0 allocations: 0 bytes)
DateTime("sat, 12 Nov 2016 07:45:36", $(Dates.RFC1123Format)) # 184.501 ns (0 allocations: 0 bytes) vs. 206.964 ns (0 allocations: 0 bytes)
DateTime("sAt, 12 Nov 2016 07:45:36", $(Dates.RFC1123Format)) # 272.136 ns (3 allocations: 120 bytes) vs. 301.229 ns (3 allocations: 120 bytes)

@gbaraldi
Copy link
Member Author

The bisect is at least confirmed? It might an inlining change or something. I didn't take too deep a look for now.

@KristofferC KristofferC added this to the 1.10 milestone Jul 24, 2023
vchuravy pushed a commit that referenced this issue Jul 27, 2023
Fixes #50612

The issue here was the reinterpret change made a bunch of operations
like `Core.bitcast(UInt64,24)` not fold, even though they are fully
known at compile time. That made `UInt32(Char)` not inline which then
caused the regression.
KristofferC pushed a commit that referenced this issue Jul 28, 2023
Fixes #50612

The issue here was the reinterpret change made a bunch of operations
like `Core.bitcast(UInt64,24)` not fold, even though they are fully
known at compile time. That made `UInt32(Char)` not inline which then
caused the regression.

(cherry picked from commit dc06468)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Regression in behavior compared to a previous version
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants