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

Parsing speed improvements #4297

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Pikacz
Copy link

@Pikacz Pikacz commented Sep 24, 2024

Checklist

  • If applicable, unit tests
  • If applicable, create follow-up issues for purchases-android and hybrids

Motivation

I was scrolling through your code and saw

precondition(
    !Thread.isMainThread,
    "Receipt parsing should not run on the main thread."
)

Which suggested that you care about performance of the parser. Since I wanted to improve my profiling skills here we are :)

Issue: #4006

Description

Improved performance of parsing receipt (which is mostly parsing of dates).

From profiler I saw that most of the time is spent in ArraySlice.toDate().
By removing dateString = String and using system Calendar I was able to sigificently speed up the process.

As fallback I left previous ISO8601DateFormatter.default.date

Example results on iPhone 8

Fastest before changes Slowest after changes Improvement
Time measured via Date 0.0146 0.0035 4 times faster
Cycle measured via mach_absolute_time 352095 85862 4 times faster
Profiler counters Cycles 36 953 516 6 863 165 5 times faster
Profiler counters INST_ALL 76 994 013 7 666 052 10 times less
Profiler counters L1D_TLB_MISS 89 366 17 531 5 times less

I've also added some additional test cases based on example receipts I was able to find through internet.

Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks so much for doing this!

return calendar
}()

private func toDateTryFastParsing() -> Date? {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dumb little nitpicks that you can totally disregard:

  • private here is redundant in that the extension is already private, we try not to repeat
    We usually add line breaks after extension definitions and before they end

See examples in the style guide

Also, given that the method already returns an optional type, it feels like the try part is also redundant, I'd maybe go with toDateFastParse

Again, I don't feel particularly strongly about any of these tbh

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Please let me know if there is anything else you wish me to change.

Redundant private
Rename
swiftlint trailing_whitespace
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants