From e5ab6a65881c6d98d4cc26483ee450a348f5a16d Mon Sep 17 00:00:00 2001 From: Edward Welbourne Date: Mon, 25 Nov 2024 12:28:07 +0100 Subject: [PATCH] Shortcut QDateTime comparison when difference is large We want to avoid caling toMSecsSinceEpoch() since it's expensive for LocalTime (which is presumed to be the common case). We can do so when both sides have the same offset from UTC (and this can cheaply be determined) but that's no help for two local times months apart, one in DST the other not. However, in this case, the difference in millis is big enough that no plausible difference in offset can overcome it, so we can again avoid toMSecsSinceEpoch() and simply compare millis. This should make some previously-expensive comparisons cheap. Add test-cases to the QDateTime ordering test that verify this doesn't lead to mis-comparison at the biggest offset-difference known. Fixes: QTBUG-131491 Change-Id: I1afd5d058c8663c908f898d4c50d0837549b87db Reviewed-by: Christian Ehrlicher (cherry picked from commit ef540d77751e24fe0b345694f43cdafca3434c68) Reviewed-by: Qt Cherry-pick Bot --- src/corelib/time/qdatetime.cpp | 33 ++++++++++++++++--- .../corelib/time/qdatetime/tst_qdatetime.cpp | 25 ++++++++++++++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/src/corelib/time/qdatetime.cpp b/src/corelib/time/qdatetime.cpp index 1b727e1c222..b41edc97456 100644 --- a/src/corelib/time/qdatetime.cpp +++ b/src/corelib/time/qdatetime.cpp @@ -3141,7 +3141,7 @@ static inline Qt::TimeSpec getSpec(const QDateTimeData &d) /* True if we *can cheaply determine* that a and b use the same offset. If they use different offsets or it would be expensive to find out, false. Calls to toMSecsSinceEpoch() are expensive, for these purposes. - See QDateTime's comparison operators. + See QDateTime's comparison operators and areFarEnoughApart(). */ static inline bool usesSameOffset(const QDateTimeData &a, const QDateTimeData &b) { @@ -3168,6 +3168,26 @@ static inline bool usesSameOffset(const QDateTimeData &a, const QDateTimeData &b Q_UNREACHABLE_RETURN(false); } +/* Even datetimes with different offset can be ordered by their getMSecs() + provided the difference is bigger than the largest difference in offset we're + prepared to believe in. Technically, it may be possible to construct a zone + with an offset outside the range and get wrong results - but the answer to + someone doing that is that their contrived timezone and its consequences are + their own responsibility. + + If two datetimes' millis lie within the offset range of one another, we can't + take any short-cuts, even if they're in the same zone, because there may be a + zone transition between them. (The full 32-hour difference would only arise + before 1845, for one date-time in The Philippines, the other in Alaska.) +*/ +bool areFarEnoughApart(qint64 leftMillis, qint64 rightMillis) +{ + constexpr qint64 UtcOffsetMillisRange + = (QTimeZone::MaxUtcOffsetSecs - QTimeZone::MinUtcOffsetSecs) * MSECS_PER_SEC; + qint64 gap = 0; + return qSubOverflow(leftMillis, rightMillis, &gap) || qAbs(gap) > UtcOffsetMillisRange; +} + // Refresh the LocalTime or TimeZone validity and offset static void refreshZonedDateTime(QDateTimeData &d, const QTimeZone &zone, QDateTimePrivate::TransitionOptions resolve) @@ -5194,8 +5214,10 @@ bool QDateTime::equals(const QDateTime &other) const if (!other.isValid()) return false; - if (usesSameOffset(d, other.d)) - return getMSecs(d) == getMSecs(other.d); + const qint64 thisMs = getMSecs(d); + const qint64 yourMs = getMSecs(other.d); + if (usesSameOffset(d, other.d) || areFarEnoughApart(thisMs, yourMs)) + return thisMs == yourMs; // Convert to UTC and compare return toMSecsSinceEpoch() == other.toMSecsSinceEpoch(); @@ -5241,8 +5263,9 @@ Qt::weak_ordering compareThreeWay(const QDateTime &lhs, const QDateTime &rhs) if (!rhs.isValid()) return Qt::weak_ordering::greater; // we know that lhs is valid here - if (usesSameOffset(lhs.d, rhs.d)) - return Qt::compareThreeWay(getMSecs(lhs.d), getMSecs(rhs.d)); + const qint64 lhms = getMSecs(lhs.d), rhms = getMSecs(rhs.d); + if (usesSameOffset(lhs.d, rhs.d) || areFarEnoughApart(lhms, rhms)) + return Qt::compareThreeWay(lhms, rhms); // Convert to UTC and compare return Qt::compareThreeWay(lhs.toMSecsSinceEpoch(), rhs.toMSecsSinceEpoch()); diff --git a/tests/auto/corelib/time/qdatetime/tst_qdatetime.cpp b/tests/auto/corelib/time/qdatetime/tst_qdatetime.cpp index 6975a4d9ced..5b8e35b963c 100644 --- a/tests/auto/corelib/time/qdatetime/tst_qdatetime.cpp +++ b/tests/auto/corelib/time/qdatetime/tst_qdatetime.cpp @@ -2537,6 +2537,31 @@ void tst_QDateTime::ordering_data() generateRow(epochEast1h, epochWest1h, Qt::weak_ordering::equivalent); if (epochTimeType == LocalTimeIsUtc) generateRow(epoch, local1970, Qt::weak_ordering::equivalent); + +#if QT_CONFIG(timezone) + // See qtimezone.h's comments on M(ax|in)UtcOffsetSecs: + QTimeZone alaska("America/Metlakatla"), phillip("Asia/Manila"); + if (alaska.isValid() && phillip.isValid()) { + // Date Narciso Claveria ordered Manila's transition: + QDateTime edict(QDate(1844, 8, 16), QTime(8, 4), phillip); // GMT start of next day + // Backends may lack relevant data, so check: + const int alaskaOffset = edict.toTimeZone(alaska).offsetFromUtc(); + const int manilaOffset = edict.toTimeZone(phillip).offsetFromUtc(); + if (manilaOffset < 0 && alaskaOffset > 0) { + qint64 offsetGap = alaskaOffset - manilaOffset; // 31h 9m 42s + QDateTime equiv = edict.toTimeZone(alaska); // 15:13:42, on the next day. + QTest::newRow("extreme.equivalent") + << edict << equiv << Qt::weak_ordering::equivalent; + // Despite internal msecs values; edict's is offsetGap * 1000 less than equiv's. + // Even the least increase in that gap does imply less: + QTest::newRow("extreme.less") + << edict << equiv.addMSecs(1) << Qt::weak_ordering::less; + // Until offsetGap seconds later, edict's msecs doesn't catch up with equiv's: + QTest::newRow("extreme.greater") + << edict.addSecs(offsetGap - 1) << equiv << Qt::weak_ordering::greater; + } + } +#endif } void tst_QDateTime::ordering()