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

Continuation of #1097 #1108

Merged
merged 3 commits into from
Jan 2, 2022
Merged

Continuation of #1097 #1108

merged 3 commits into from
Jan 2, 2022

Conversation

icambron
Copy link
Member

@icambron icambron commented Jan 2, 2022

Continuing #1097

@gpaucot Added some commits:

  1. I really wanted to push the work into toSQLDate and toSQLTime. So I set up both your fully-inlined one and my refactor and benchmarked them against each other, then tweaked mine until it was within the benchmark's noise level.
  2. Admittedly, my code benefited from some tweaks to the logic, and once I backported those into the inlined code, that code got faster again. But only by a little, so I'm going to live with it.
  3. I also removed some no-longer-needed code and did some other cleanup

These were the logic changes relative to your PR:

  1. Turns out, we're supposed to always use 6-digit years for negative years. That's actually DateTime toISO pads to 4 instead of 6 digits for negative years #951. That has a small perf advantage because it's quicker to compare o.c.year > 9999 || o.c.year < 0 than it is to do Math.abs(o.c.year) > 9999. So we'll squash that bug by-the-by.
  2. You had logic for allowZ, but that's not actually an option on toISO. You always get Z for the UTC zone; the old code hardcoded into the call to the formatter. No real perf difference either way

@icambron
Copy link
Member Author

icambron commented Jan 2, 2022

Actually just going to merge this. More perf changes can get tacked on on top of it. Thanks for your help @gpaucot!

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