From b7fe1ca68aa83fd512b568719e39d2abc2adfb84 Mon Sep 17 00:00:00 2001 From: Balazs Varga Date: Tue, 2 Jun 2026 08:57:37 +0200 Subject: [PATCH] tdf#164843 - sc: fix ScRange hash so SubTotal re-listening stays fast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Proper fix for the slowness originally reported in tdf#164843: on a sheet with many SubTotal formulas over a relative named range, deleting a named range hangs for ~2 min 50 sec (on a debug build). tdf#164843 was first "fixed" in b89047a0f0100fb30121084cf42815aa792c1f88 by trimming the SubTotal double references to the data area at compile time. That mutated the stored reference - the formula was rewritten and rows added later were dropped - which got reported as tdf#168013 and fixed by moving the trim to evaluation time in the preceding commit 7ade1318a646c99926b5685d40e1e4174454bf4d ("sc: don't mutate SubTotal reference when trimming"). Removing the compile-time trim, however, brought the original tdf#164843 slowness back: the trim had only been masking the real cause. That cause is in ScRange::hashArea() / hashStartColumn(): each corner was packed into a fixed high bit-field and the fields XORed together, so only the end row reached the low-order bits that determine the bucket index. Ranges that share their end corner but differ only in the start corner - exactly what a relative named range yields (A2000:A, A2001:A, ...) - all hashed into a single bucket. ScBroadcastAreaSlot's unordered_set then degraded to O(n) per insert, so re-listening every formula after the name-range change was O(n^2): ~2 min 50 s, with the broadcast slot collapsed into one non-empty bucket holding ~1000 areas. Combine the corners with o3tl::hash_combine so every one influences the low-order bits. The bucket distribution goes from 1 bucket of 1000 to 1000 buckets of 1, and the re-listen drops from ~2 min 50 sec to 2-3 sec. hashStartColumn() (used by the lookup and range caches) had the identical defect and gets the same fix. follow-up of: 7ade1318a646c99926b5685d40e1e4174454bf4d Change-Id: I293f7e108cd2c788b159451061b547a11ce748bf Signed-off-by: Balazs Varga Reviewed-on: https://gerrit.collaboraoffice.com/c/online/+/3654 Reviewed-by: Caolán McNamara Tested-by: Jenkins CPCI (cherry picked from commit c4509310d0fcafe20397659ee6b6911f21d30bec) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/206167 Reviewed-by: Xisco Fauli Tested-by: Jenkins --- sc/inc/address.hxx | 62 ++++++++++++++++---------------------- sc/qa/unit/ucalc_range.cxx | 25 +++++++++++++++ 2 files changed, 51 insertions(+), 36 deletions(-) diff --git a/sc/inc/address.hxx b/sc/inc/address.hxx index b8a34a887752..eb61946dc679 100644 --- a/sc/inc/address.hxx +++ b/sc/inc/address.hxx @@ -19,6 +19,7 @@ #pragma once +#include #include #include @@ -728,46 +729,35 @@ inline bool ScRange::Intersects( const ScRange& rRange ) const inline size_t ScRange::hashArea() const { -#if SAL_TYPES_SIZEOFPOINTER == 8 - // 12 bits for the columns and 20 bits for the rows - return - (static_cast(aStart.Row()) << 44) ^ - (static_cast(aStart.Col()) << 32) ^ - (static_cast(aEnd.Col()) << 20) ^ - static_cast(aEnd.Row()); -#else - // Assume that there are not that many ranges with identical corners so we - // won't have too many collisions. Also assume that more lower row and - // column numbers are used so that there are not too many conflicts with - // the columns hashed into the values, and that start row and column - // usually don't exceed certain values. High bits are not masked off and - // may overlap with lower bits of other values, e.g. if start column is - // greater than assumed. - return - (static_cast(aStart.Row()) << 26) ^ // start row <= 2^6 - (static_cast(aStart.Col()) << 21) ^ // start column <= 2^5 - (static_cast(aEnd.Col()) << 15) ^ // end column <= 2^6 - static_cast(aEnd.Row()); // end row <= 2^15 -#endif + // Mix all four corners into the hash so each one affects its low-order bits, + // which are what determine the bucket index in a hash table. The previous + // shift+xor parked the start column/row in high bits, so ranges that share + // the same end corner but differ only in the start corner - e.g. a relative + // named range resolving to A1:X, A2:X, A3:X, ... or repeated large LOOKUP() + // areas - all hashed to a single bucket, turning unordered_set insert/find + // into O(n). tdf#164843 + size_t h = 0; + o3tl::hash_combine(h, aStart.Row()); + o3tl::hash_combine(h, aStart.Col()); + o3tl::hash_combine(h, aEnd.Col()); + o3tl::hash_combine(h, aEnd.Row()); + return h; } inline size_t ScRange::hashStartColumn() const { -#if SAL_TYPES_SIZEOFPOINTER == 8 - // 20 bits for the rows - return - (static_cast(aStart.Col()) << 40) ^ - (static_cast(aStart.Row()) << 20) ^ - static_cast(aEnd.Row()); -#else - // Assume that for the start row more lower row numbers are used so that - // there are not too many conflicts with the column hashed into the higher - // values. - return - (static_cast(aStart.Col()) << 24) ^ // start column <= 2^8 - (static_cast(aStart.Row()) << 16) ^ // start row <= 2^8 - static_cast(aEnd.Row()); -#endif + // As in hashArea(): mix the fields so each one affects the low-order bits + // that determine the bucket index. The previous shift+xor parked the start + // column/row in high bits, so ranges that share the same end row but differ + // only in the start corner hashed to a single bucket and made the lookup and + // range caches O(n) per probe. Keyed on the start column and the row span + // (end column intentionally omitted - lookups are on the first column). + // tdf#164843 + size_t h = 0; + o3tl::hash_combine(h, aStart.Col()); + o3tl::hash_combine(h, aStart.Row()); + o3tl::hash_combine(h, aEnd.Row()); + return h; } [[nodiscard]] inline bool ValidRange( const ScRange& rRange, SCCOL nMaxCol, SCROW nMaxRow ) diff --git a/sc/qa/unit/ucalc_range.cxx b/sc/qa/unit/ucalc_range.cxx index e118a039f79e..53dbab9a798c 100644 --- a/sc/qa/unit/ucalc_range.cxx +++ b/sc/qa/unit/ucalc_range.cxx @@ -18,6 +18,8 @@ #include #include +#include + class ScRangeTest : public ScUcalcTestBase { }; @@ -71,6 +73,29 @@ CPPUNIT_TEST_FIXTURE(ScRangeTest, testTdf147451) CPPUNIT_ASSERT_MESSAGE("Should fail to parse.", !(nRes & ScRefFlags::VALID)); } +CPPUNIT_TEST_FIXTURE(ScRangeTest, testHashDistribution) +{ + // tdf#164843: ranges sharing an end corner but differing only in the start + // corner must spread across hash buckets. The broken hash put all of them in + // a single bucket (distinct buckets == 1); a good hash spreads ~1000 ranges + // across most of the 1024 buckets. + constexpr size_t nCount = 1000; + constexpr size_t nBuckets = 1024; + + std::unordered_set aAreaBuckets; + std::unordered_set aStartColBuckets; + for (size_t i = 0; i < nCount; ++i) + { + // varying start row, shared end row (near MAXROW) + const ScRange aRange(28, static_cast(2000 + i), 0, 28, 1048575, 0); + aAreaBuckets.insert(aRange.hashArea() % nBuckets); + aStartColBuckets.insert(aRange.hashStartColumn() % nBuckets); + } + + CPPUNIT_ASSERT_GREATER(nCount / 2, aAreaBuckets.size()); + CPPUNIT_ASSERT_GREATER(nCount / 2, aStartColBuckets.size()); +} + class ScRangeUpdaterTest : public CppUnit::TestFixture { public: