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"); +}