From 1cd3a6f45f3555d23b509d2e4ae7de41b944c240 Mon Sep 17 00:00:00 2001 From: russimicro Date: Wed, 5 Aug 2026 08:26:21 -0500 Subject: [PATCH] perf(abi): AdsGetKeyCount no recuenta las claves vivas en cada pintada Con SET DELETED ON el conteo de claves de un orden cuesta un deleted_at() por entrada, y deleted_at() lee el registro completo: sobre ARTICULO (34.595 filas, 1,6 KB por fila) son ~15 ms y ~54 MB leidos. AdsGetKeyCount es justo lo que un TXBrowse pide en CADA pintada para dimensionar la barra, asi que el browse pagaba ese precio una y otra vez por una respuesta que no habia cambiado: MantenArt sobre una compania ADT tardaba ~5 s en abrir y en reordenar. Se memoriza el conteo por indice, con la generacion de la tabla como clave. La generacion avanza en delete / recall / append / zap / pack y en el refresh que hace visible el trabajo de otro puesto, que son los unicos momentos en que el conjunto de registros vivos puede cambiar. Medido sobre la 0Y real (ADI de 22 tags): 50 llamadas pasan de 750 ms a 0 ms. Aplica igual a CDX, NTX y ADI, que compartian el mismo camino. Co-Authored-By: Claude Opus 5 (1M context) --- src/abi/ace_exports.cpp | 36 ++++++++-- src/engine/table.cpp | 5 ++ src/engine/table.h | 14 ++++ tests/CMakeLists.txt | 1 + tests/unit/abi_keycount_cache_test.cpp | 94 ++++++++++++++++++++++++++ 5 files changed, 145 insertions(+), 5 deletions(-) create mode 100644 tests/unit/abi_keycount_cache_test.cpp diff --git a/src/abi/ace_exports.cpp b/src/abi/ace_exports.cpp index 1ce9dc15..5905b8cf 100644 --- a/src/abi/ace_exports.cpp +++ b/src/abi/ace_exports.cpp @@ -9963,8 +9963,33 @@ 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& walk) { + const std::vector& 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 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 by_recno(walk); std::sort(by_recno.begin(), by_recno.end()); std::uint32_t n = 0; @@ -9972,6 +9997,7 @@ std::uint32_t count_live_recnos(openads::engine::Table* t, 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( 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( cdx->ordered_recnos_cached().size()); @@ -35132,7 +35158,7 @@ UNSIGNED32 ENTRYPOINT AdsGetKeyCount(ADSHANDLE hIndex, UNSIGNED16 /*usFilter*/, dynamic_cast(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( ntx->ordered_recnos_cached().size()); @@ -35144,7 +35170,7 @@ UNSIGNED32 ENTRYPOINT AdsGetKeyCount(ADSHANDLE hIndex, UNSIGNED16 /*usFilter*/, dynamic_cast(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( adi->ordered_recnos_cached().size()); diff --git a/src/engine/table.cpp b/src/engine/table.cpp index 35d6d2e3..fd20869e 100644 --- a/src/engine/table.cpp +++ b/src/engine/table.cpp @@ -965,6 +965,7 @@ Table::read_field(std::uint16_t field_index) { } util::Result 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 Table::apply_tx_rollback_append(std::uint32_t recno) { } util::Result 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 Table::mark_deleted() { } util::Result 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 Table::deleted_at(std::uint32_t recno) { } util::Result Table::zap() { + bump_live_gen(); if (mode_ == OpenMode::Read) { return util::Error{5000, 0, "table opened read-only", ""}; } @@ -1371,6 +1375,7 @@ util::Result Table::rollback_appends(std::vector recnos) { } util::Result Table::pack() { + bump_live_gen(); if (mode_ == OpenMode::Read) { return util::Error{5000, 0, "table opened read-only", ""}; } diff --git a/src/engine/table.h b/src/engine/table.h index 2812effe..7de60e15 100644 --- a/src/engine/table.h +++ b/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 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> 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; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d1fbb3ec..4e6e89a2 100644 --- a/tests/CMakeLists.txt +++ b/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 diff --git a/tests/unit/abi_keycount_cache_test.cpp b/tests/unit/abi_keycount_cache_test.cpp new file mode 100644 index 00000000..0be57f20 --- /dev/null +++ b/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 +#include +#include + +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"); +}