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

Adopt ISO8601FormatStyle on newer platforms #637

Open
czechboy0 opened this issue Sep 30, 2024 · 8 comments
Open

Adopt ISO8601FormatStyle on newer platforms #637

czechboy0 opened this issue Sep 30, 2024 · 8 comments
Assignees
Labels
area/runtime Affects: the runtime library. kind/feature New feature. size/S Small task. (A couple of hours of work.)
Milestone

Comments

@czechboy0
Copy link
Contributor

czechboy0 commented Sep 30, 2024

Motivation

ISO8601FormatStyle is significantly faster than ISO8601DateFormatter.

Proposed solution

Use #available to dynamically use the faster and more modern implementation in ISO8601FormatStyle, fall back to ISO8601DateFormatter otherwise (today's behavior).

We should also add benchmarks to the runtime package while we're there to measure the improvement.

Alternatives considered

No response

Additional information

No response

@czechboy0 czechboy0 added kind/feature New feature. status/triage Collecting information required to triage the issue. size/S Small task. (A couple of hours of work.) and removed status/triage Collecting information required to triage the issue. labels Sep 30, 2024
@czechboy0 czechboy0 added this to the Post-1.0 milestone Sep 30, 2024
@simonjbeaumont simonjbeaumont added the area/runtime Affects: the runtime library. label Sep 30, 2024
@arthurcro
Copy link
Contributor

Hey! I would be happy to help with this if it's not already picked up! 😊

@czechboy0
Copy link
Contributor Author

Thank you @arthurcro - go ahead! 👍

@simonjbeaumont
Copy link
Contributor

@arthurcro thanks for getting stuck in! Just a note that we don't currently have benchmarks in the repo, and we're about to migrate the CI to GitHub Actions.

This shouldn't block you working on the issue, but it might delay us landing it, as we'd like to get this in place. I'll try and schedule that work this week.

@simonjbeaumont
Copy link
Contributor

@arthurcro I expedited this to ensure you're unblocked to work on this when you get to it. The CI is updated. We have an open PR to add package benchmarks: apple/swift-openapi-runtime#119.

And it looks like this should be a good patch...

========================
OpenAPIRuntimeBenchmarks
========================

Date.ISO8601Format(_:)
╒═════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│ Metric                  │      p0 │     p25 │     p50 │     p75 │     p90 │     p99 │    p100 │ Samples │
╞═════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│ Malloc (total) *        │       2 │       2 │       2 │       2 │       2 │       2 │       2 │       5 │
├─────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (total CPU) (ns) * │     771 │     773 │     773 │     782 │     794 │     794 │     794 │       5 │
╘═════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛

ISO8601DateFormatter.string(from:)
╒═════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│ Metric                  │      p0 │     p25 │     p50 │     p75 │     p90 │     p99 │    p100 │ Samples │
╞═════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│ Malloc (total) *        │       9 │       9 │       9 │       9 │       9 │       9 │       9 │       5 │
├─────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (total CPU) (ns) * │    1536 │    1547 │    1566 │    1574 │    1606 │    1606 │    1606 │       5 │
╘═════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛

ISO8601DateTranscoder.encode(_:)
╒═════════════════════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╤═════════╕
│ Metric                  │      p0 │     p25 │     p50 │     p75 │     p90 │     p99 │    p100 │ Samples │
╞═════════════════════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╪═════════╡
│ Malloc (total) *        │       9 │       9 │       9 │       9 │       9 │       9 │       9 │       5 │
├─────────────────────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┼─────────┤
│ Time (total CPU) (ns) * │    1528 │    1529 │    1530 │    1541 │    1558 │    1558 │    1558 │       5 │
╘═════════════════════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╧═════════╛

As part of the work, it would be good to remove those benchmarks against foundation, once we have comparable numbers for the ISO8601DateTranscoder.

@arthurcro
Copy link
Contributor

Thank you @simonjbeaumont, I appreciate it! I will start on this issue as soon as possible. I'll make sure to also update the benchmarks and remove the ones against foundation.

@arthurcro
Copy link
Contributor

Hey @czechboy0, I made an early draft MR on my fork but while running the tests I noticed testDateTranscoder_iso8601WithFractionalSeconds is failing with:

testDateTranscoder_iso8601WithFractionalSeconds(): XCTAssertEqual failed: ("2023-01-18T10:04:11.122") is not equal to ("2023-01-18T10:04:11.123Z")

I tried reproducing the issue in a Swift playground, and got the same results. Also, parsing the formatted date with the same Date.ISO8601FormatStyle gives me a different date.
I wonder if that's expected or might be a Foundation issue. 🤔

let initialDate = Date(timeIntervalSince1970: 1_674_036_251.123) 
let formatStyle = Date.ISO8601FormatStyle.iso8601
    .year()
    .month()
    .day()
    .time(includingFractionalSeconds: true)

let encoded = initialDate.formatted(formatStyle)
print(encoded) // prints 2023-01-18T10:04:11.122 instead of 2023-01-18T10:04:11.123
print(initialDate == (try! formatStyle.parse(encoded)) // prints false

@czechboy0
Copy link
Contributor Author

@arthurcro that's interesting, can you open an issue against https://github.com/swiftlang/swift-foundation if you manage to reproduce it on a minimal project that doesn't include Swift OpenAPI Generator?

@arthurcro
Copy link
Contributor

@czechboy0 definitely, it's reproducible in a brand new Swift playground with only the code snippet I charged. I just opened an issue here swiftlang/swift-foundation#963.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/runtime Affects: the runtime library. kind/feature New feature. size/S Small task. (A couple of hours of work.)
Projects
None yet
Development

No branches or pull requests

3 participants