perf(abi): AdsGetKeyCount does not recount live keys on every paint

src/abi/ace_exports.cpp

@@ -9963,15 +9963,41 @@ SIGNED32 to_julian(int y, int m, int d) {
 // of the same 34,595-row table: 14 ms where key order happened to follow recno,
 // 492 ms where it did not. The count does not depend on the order, so read the
 // records in the order the file stores them.
+// Counting the live entries of an index costs one deleted_at() per entry, and
+// deleted_at() reads the whole record. On a 34.595-row table that is ~15 ms,
+// and AdsGetKeyCount is what a TXBrowse asks on EVERY paint to size its
+// scrollbar -- so the browse paid it again and again for an answer that had
+// not changed. Memoise per index (`owner`), keyed by the table's live
+// generation, which moves on delete / recall / append / zap / pack and on the
+// refresh that makes another station's work visible.
 std::uint32_t count_live_recnos(openads::engine::Table* t,
-                                const std::vector<std::uint32_t>& walk) {
+                                const std::vector<std::uint32_t>& walk,
+                                const void* owner) {
+    struct Cached {
+        const openads::engine::Table* table = nullptr;
+        std::uint64_t                 gen   = 0;
+        std::size_t                   size  = 0;
+        std::uint32_t                 count = 0;
+    };
+    static std::unordered_map<const void*, Cached> cache;
+
+    const std::uint64_t gen = t->live_gen();
+    if (owner != nullptr) {
+        auto it = cache.find(owner);
+        if (it != cache.end() && it->second.table == t &&
+            it->second.gen == gen && it->second.size == walk.size()) {
+            return it->second.count;
+        }
+    }
+
     std::vector<std::uint32_t> by_recno(walk);
     std::sort(by_recno.begin(), by_recno.end());
     std::uint32_t n = 0;
     for (std::uint32_t rn : by_recno) {
         auto del = t->deleted_at(rn);
         if (del && !del.value()) ++n;
     }
+    if (owner != nullptr) cache[owner] = Cached{t, gen, walk.size(), n};
     return n;
 }
 
@@ -10135,7 +10161,7 @@ UNSIGNED32 ENTRYPOINT AdsGetRecordCount(ADSHANDLE hTable, UNSIGNED16 bFilterOpti
                         live_p);
                 } else if (hide_del) {
                     *pulRecordCount = count_live_recnos(
-                        t, cdx->ordered_recnos_cached());
+                        t, cdx->ordered_recnos_cached(), cdx);
                 } else {
                     *pulRecordCount = static_cast<UNSIGNED32>(
                         cdx->ordered_recnos_cached().size());
@@ -35120,7 +35146,7 @@ UNSIGNED32 ENTRYPOINT AdsGetKeyCount(ADSHANDLE hIndex, UNSIGNED16 /*usFilter*/,
                     sc.bottom.value_or(""),
                     live_p);
             } else if (hide_del) {
-                *pulCount = count_live_recnos(t, cdx->ordered_recnos_cached());
+                *pulCount = count_live_recnos(t, cdx->ordered_recnos_cached(), cdx);
             } else {
                 *pulCount = static_cast<UNSIGNED32>(
                     cdx->ordered_recnos_cached().size());
@@ -35132,7 +35158,7 @@ UNSIGNED32 ENTRYPOINT AdsGetKeyCount(ADSHANDLE hIndex, UNSIGNED16 /*usFilter*/,
                 dynamic_cast<openads::drivers::ntx::NtxIndex*>(ord->index())) {
             const bool hide_del = !t->show_deleted_records();
             if (hide_del) {
-                *pulCount = count_live_recnos(t, ntx->ordered_recnos_cached());
+                *pulCount = count_live_recnos(t, ntx->ordered_recnos_cached(), ntx);
             } else {
                 *pulCount = static_cast<UNSIGNED32>(
                     ntx->ordered_recnos_cached().size());
@@ -35144,7 +35170,7 @@ UNSIGNED32 ENTRYPOINT AdsGetKeyCount(ADSHANDLE hIndex, UNSIGNED16 /*usFilter*/,
                 dynamic_cast<openads::drivers::adi::AdiIndex*>(ord->index())) {
             const bool hide_del = !t->show_deleted_records();
             if (hide_del) {
-                *pulCount = count_live_recnos(t, adi->ordered_recnos_cached());
+                *pulCount = count_live_recnos(t, adi->ordered_recnos_cached(), adi);
             } else {
                 *pulCount = static_cast<UNSIGNED32>(
                     adi->ordered_recnos_cached().size());

src/engine/table.cpp

@@ -965,6 +965,7 @@ Table::read_field(std::uint16_t field_index) {
 }
 
 util::Result<void> Table::append_record() {
+    bump_live_gen();
     if (mode_ == OpenMode::Read) {
         return util::Error{5000, 0, "table opened read-only", ""};
     }
@@ -1254,6 +1255,7 @@ util::Result<void> Table::apply_tx_rollback_append(std::uint32_t recno) {
 }
 
 util::Result<void> Table::mark_deleted() {
+    bump_live_gen();
     if (state_ != State::Positioned) {
         // rddads (Harbour contrib RDD) special-cases 5068 (AE_NO_CURRENT_RECORD)
         // to return blank field values at BOF/EOF; 5026 causes a hard error.
@@ -1267,6 +1269,7 @@ util::Result<void> Table::mark_deleted() {
 }
 
 util::Result<void> Table::recall_deleted() {
+    bump_live_gen();
     if (state_ != State::Positioned) {
         // rddads (Harbour contrib RDD) special-cases 5068 (AE_NO_CURRENT_RECORD)
         // to return blank field values at BOF/EOF; 5026 causes a hard error.
@@ -1293,6 +1296,7 @@ util::Result<bool> Table::deleted_at(std::uint32_t recno) {
 }
 
 util::Result<void> Table::zap() {
+    bump_live_gen();
     if (mode_ == OpenMode::Read) {
         return util::Error{5000, 0, "table opened read-only", ""};
     }
@@ -1371,6 +1375,7 @@ util::Result<void> Table::rollback_appends(std::vector<std::uint32_t> recnos) {
 }
 
 util::Result<void> Table::pack() {
+    bump_live_gen();
     if (mode_ == OpenMode::Read) {
         return util::Error{5000, 0, "table opened read-only", ""};
     }

src/engine/table.h

@@ -146,6 +146,9 @@ class Table {
     // connections become visible.  Called by the server before answering
     // GetRecordCount and by AdsRefreshRecord.
     void refresh_record_count_from_disk() noexcept {
+        // A refresh is the moment another station's appends and deletes
+        // become visible, so any cached live count taken before it is stale.
+        bump_live_gen();
         driver_->refresh_record_count_from_disk();
     }
     // Clipper / SAP-ACE convention: phantom position past last
@@ -236,6 +239,16 @@ class Table {
     // call, with no reuse between calls.
     util::Result<bool> deleted_at(std::uint32_t recno);
 
+    // Bumped whenever the set of live records can have changed: a delete, a
+    // recall, an append, a zap/pack, or a refresh that saw another station's
+    // work. Callers that cache a live-record count (AdsGetKeyCount, whose
+    // count costs one deleted_at per index entry) keep this value alongside
+    // the cached number and recount only when it moves. Without that cache a
+    // TXBrowse over a 34k-row table spent ~15 ms per paint counting the same
+    // rows again.
+    std::uint64_t live_gen() const noexcept { return live_gen_; }
+    void bump_live_gen() noexcept { ++live_gen_; }
+
     // True only when a concrete record is loaded (not BOF/EOF/Limbo).
     bool positioned() const noexcept { return state_ == State::Positioned; }
 
@@ -581,6 +594,7 @@ class Table {
     // current row; applied once by commit_dirty_record().
     std::vector<std::pair<drivers::IIndex*, std::string>> index_snap_;
     bool                                          cache_enabled_  = false;
+    std::uint64_t                                 live_gen_       = 0;
     bool                                          last_seek_found_ = false;
     bool                                          aof_active_      = false;
     bool                                          transaction_free_ = false;

tests/CMakeLists.txt

@@ -243,6 +243,7 @@ add_executable(openads_unit_tests
     unit/abi_adt_scope_validation_test.cpp
     unit/abi_adi_smoke_test.cpp
     unit/abi_adi_keycount_test.cpp
+    unit/abi_keycount_cache_test.cpp
     unit/abi_adi_tagdir_order_test.cpp
     unit/abi_adi_tagdir_wide_page_test.cpp
     unit/abi_adt_sql_test.cpp

tests/unit/abi_keycount_cache_test.cpp

@@ -0,0 +1,94 @@
+// AdsGetKeyCount memoises the live-key count per index (counting it costs one
+// deleted_at per index entry, and a TXBrowse asks on every paint). The cache
+// is keyed by the table's live generation, so this pins that the generation
+// actually moves: a delete, a recall and an append must each be reflected in
+// the very next call, on CDX and on ADI alike.
+#include "doctest.h"
+#include "openads/ace.h"
+
+#include <cstring>
+#include <filesystem>
+#include <string>
+
+namespace fs = std::filesystem;
+
+namespace {
+
+UNSIGNED32 key_count(ADSHANDLE hIdx) {
+    UNSIGNED32 c = 0;
+    REQUIRE(AdsGetKeyCount(hIdx, ADS_RESPECTFILTERS, &c) == AE_SUCCESS);
+    return c;
+}
+
+void run_case(const char* tag, UNSIGNED16 tabletype, const char* idxfile) {
+    fs::path tmp = fs::temp_directory_path() / (std::string("oads_kcache_") + tag);
+    { std::error_code ec; fs::remove_all(tmp, ec); fs::create_directories(tmp, ec); }
+
+    UNSIGNED8 srv[260]{};
+    std::memcpy(srv, tmp.string().c_str(), tmp.string().size());
+    ADSHANDLE hConn = 0;
+    REQUIRE(AdsConnect60(srv, ADS_LOCAL_SERVER, nullptr, nullptr, 0, &hConn)
+            == AE_SUCCESS);
+
+    UNSIGNED8 tbl[]    = "kc";
+    UNSIGNED8 flddef[] = "Name,Character,20";
+    ADSHANDLE hTable   = 0;
+    REQUIRE(AdsCreateTable(hConn, tbl, nullptr, tabletype, ADS_ANSI, 0, 0, 0,
+                           flddef, &hTable) == AE_SUCCESS);
+
+    UNSIGNED8 fld[] = "Name";
+    for (int i = 0; i < 20; ++i) {
+        char v[8];
+        std::snprintf(v, sizeof(v), "NAME%02d", i);
+        REQUIRE(AdsAppendRecord(hTable) == AE_SUCCESS);
+        REQUIRE(AdsSetString(hTable, fld, (UNSIGNED8*)v,
+                             (UNSIGNED32)std::strlen(v)) == AE_SUCCESS);
+        REQUIRE(AdsWriteRecord(hTable) == AE_SUCCESS);
+    }
+
+    UNSIGNED8 file[64]{};
+    std::strncpy((char*)file, idxfile, sizeof(file) - 1);
+    ADSHANDLE hIdx = 0;
+    REQUIRE(AdsCreateIndex61(hTable, file, (UNSIGNED8*)"NAME",
+                            (UNSIGNED8*)"Name", nullptr, nullptr, 0, 0, &hIdx)
+            == AE_SUCCESS);
+
+    REQUIRE(AdsShowDeleted(0) == AE_SUCCESS);   // SET DELETED ON
+
+    CHECK(key_count(hIdx) == 20u);
+    CHECK(key_count(hIdx) == 20u);              // segunda: sale de la cache
+
+    // Un borrado tiene que verse en la llamada siguiente.
+    REQUIRE(AdsGotoTop(hTable) == AE_SUCCESS);
+    REQUIRE(AdsDeleteRecord(hTable) == AE_SUCCESS);
+    CHECK(key_count(hIdx) == 19u);
+
+    // Y un recall tambien. Con DELETED ON el registro borrado es invisible,
+    // asi que hay que destaparlo para poder pararse encima y recuperarlo.
+    REQUIRE(AdsShowDeleted(1) == AE_SUCCESS);
+    REQUIRE(AdsGotoRecord(hTable, 1) == AE_SUCCESS);
+    REQUIRE(AdsRecallRecord(hTable) == AE_SUCCESS);
+    REQUIRE(AdsShowDeleted(0) == AE_SUCCESS);
+    CHECK(key_count(hIdx) == 20u);
+
+    // Un append entra al indice y al conteo.
+    REQUIRE(AdsAppendRecord(hTable) == AE_SUCCESS);
+    REQUIRE(AdsSetString(hTable, fld, (UNSIGNED8*)"NAME99", 6) == AE_SUCCESS);
+    REQUIRE(AdsWriteRecord(hTable) == AE_SUCCESS);
+    CHECK(key_count(hIdx) == 21u);
+
+    REQUIRE(AdsShowDeleted(1) == AE_SUCCESS);
+    AdsCloseTable(hTable);
+    AdsDisconnect(hConn);
+    { std::error_code ec; fs::remove_all(tmp, ec); }
+}
+
+} // namespace
+
+TEST_CASE("AdsGetKeyCount cache: CDX sees delete / recall / append") {
+    run_case("cdx", ADS_CDX, "kc.cdx");
+}
+
+TEST_CASE("AdsGetKeyCount cache: ADI sees delete / recall / append") {
+    run_case("adi", ADS_ADT, "kc.adi");
+}