Skip to content

perf(engine): counting live keys no longer re-reads the table per record - #153

Closed
russimicro wants to merge 2 commits into
FiveTechSoft:mainfrom
russimicro:perf/keycount-deleted-without-goto
Closed

perf(engine): counting live keys no longer re-reads the table per record#153
russimicro wants to merge 2 commits into
FiveTechSoft:mainfrom
russimicro:perf/keycount-deleted-without-goto

Conversation

@russimicro

Copy link
Copy Markdown
Collaborator

Found while profiling a FiveWin TXBrowse over an ADT company (34,595 rows,
187 fields). It is worth saying plainly: in #145 we reported that screen as a
FiveWin problem. It was not — most of the time was this.

Problem

AdsGetKeyCount and AdsGetRecordCount (over an order handle) exclude deleted
rows while SET DELETED is ON by walking the cached index and testing each recno:

live = [t](std::uint32_t rn) {
    if (!t->goto_record(rn)) return false;
    return !t->is_deleted();
};

goto_record() opens with driver_->invalidate_read_cache(). It has to — an
absolute reposition must see what another writer just wrote. That is right for
navigation and ruinous here: the walk throws away the read-ahead block on every
iteration, so a sequential count becomes one block read per record. And nothing
is cached between calls, so every caller pays it again.

Measured on the table above (Harbour OrdKeyCount(), ADT + ADI, no scope, no
filter):

SET DELETED OFF   OrdKeyCount = 34595      9 ms      (x5 more:    0 ms)
SET DELETED ON    OrdKeyCount = 34595   1381 ms      (x5 more: 6859 ms)

The SET DELETED OFF path takes the ordered_recnos_cached().size() shortcut,
which is why this never showed up in a benchmark — ours measured with
AdsSetDeleted(.F.) and reported 9 ms for months.

It matters because FiveWin's TXBrowse evaluates KeyCount() about five times
while opening a browse (once in Adjust, once per Refresh, and Refresh runs
from Adjust, SetFocus, ReArrangeCols and RestoreState). Three seconds of
a 3.1-second screen were this call.

Change

New Table::deleted_at(recno): reads the record through the normal read path —
so the driver's read-ahead block stays warm — and does not move the cursor.

This is the same reasoning as the existing load_record_for_bulk_scan, which
already bypasses goto_record for sequential index builds; its comment
describes exactly this trap.

Both call sites (the CDX branch of AdsGetRecordCount and of AdsGetKeyCount)
now use it, including through count_scoped_keys's live callback. The
saved/restored cursor recno goes with them — nothing moves the cursor now.

After, same table, same counts:

SET DELETED ON    OrdKeyCount = 34595     14 ms      (x5 more:   72 ms)

The screen went from 3.1 s to 440 ms, below the 813 ms of the same screen over
native DBFCDX.

Verification

tests/unit/abi_keycount_deleted_scan_test.cpp pins the two invariants the
change must preserve: the counts are unchanged with SET DELETED both ways, and
counting leaves the cursor and the record buffer where they were.

To be straight about what that test does and does not do: it is a contract
guard, not a regression test — the old code saved and restored the cursor, so it
would pass too. The speed is not asserted, because a timing threshold would be
flaky on CI. The evidence for the win is the measurement above, from a
standalone Harbour harness against a real table; happy to share it.

Full suite on this branch (MSVC x64 Release): 1229/1235. The 6 failures are the
pre-existing abi_pritpal_lock_test remote-connect ones noted in the v1.8.37
release.

AdsGetKeyCount / AdsGetRecordCount over an order exclude deleted rows
while SET DELETED is ON by walking the cached index and testing each
recno. The test went through goto_record(), which invalidates the
driver's read-ahead cache on every call -- it has to, an absolute
reposition must see what another writer just wrote. That turned a
sequential count into one block read per record, on every call, with no
reuse between calls.

Measured on an ADT table of 34,595 rows:

    SET DELETED OFF   OrdKeyCount     9 ms   (x5 more:    0 ms)
    SET DELETED ON    OrdKeyCount  1381 ms   (x5 more: 6859 ms)

FiveWin's TXBrowse evaluates KeyCount() about five times while opening a
browse, so a maintenance screen paid seconds for the count alone.

Add Table::deleted_at(recno): reads the record through the normal read
path so the read-ahead block stays warm, and does not move the cursor.
This is the same reasoning as the existing load_record_for_bulk_scan,
which already bypasses goto_record for sequential scans. After:

    SET DELETED ON    OrdKeyCount    14 ms   (x5 more:   72 ms)

Same counts. The saved/restored cursor recno goes with it -- nothing
moves the cursor now.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

Keeping the read-ahead warm was only half of it. The walk arrives ordered
by KEY, and on an index whose key does not correlate with recno,
consecutive reads still land in different blocks -- every record costs a
fresh 64 KB fetch and the read-ahead buys nothing.

Two copies of the same 34,595-row table, same code, same counts, after
the previous commit:

    ADT/ADI, key order follows recno     14 ms
    DBF/CDX, key order scattered        492 ms

The count does not depend on the order, so sort the recnos and read the
records in the order the file stores them:

    ADT/ADI                              16 ms   (the sort costs ~2 ms)
    DBF/CDX                              23 ms

Non-scoped path of both entry points. The scoped path goes through
count_scoped_keys, which walks a key range and cannot be reordered
without changing what it means.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@russimicro

Copy link
Copy Markdown
Collaborator Author

Second commit pushed — the first one was correct but only half the fix, and it
took a second table to see it.

Table::deleted_at() keeps the driver's read-ahead warm, which is necessary but
not sufficient: the walk arrives ordered by key, and on an index whose key
does not correlate with recno, consecutive reads land in different blocks
anyway. Every record still costs a fresh 64 KB fetch.

We only noticed because the table we validated on was the lucky case. Two copies
of the same 34,595-row data, both after the first commit:

per call x5
ADT/ADI — key order happens to follow recno 14 ms 72 ms
DBF/CDX — key order scattered 492 ms 2,465 ms

The count does not depend on the order, so the recnos are sorted and the records
read in the order the file stores them:

per call x5
ADT/ADI 16 ms 72 ms
DBF/CDX 23 ms 108 ms

Same counts throughout. The sort costs ~2 ms on 34,595 recnos, which is why the
lucky case barely moves.

Applied to the non-scoped path of both entry points. The scoped path goes
through count_scoped_keys, which walks a key range — reordering there would
change what it means, so it is untouched.

Suite on the branch: 1228/1235. Six are the known abi_pritpal_lock_test
remote-connect failures; the seventh is abi_remote_ordered_prefetch, which is
flaky here — run in isolation it passed twice and failed once.

FiveTechSoft added a commit that referenced this pull request Jul 30, 2026
Cherry-picked and conflict-resolved against main (post CI fix 54e977c):

- #150 fix(sql): ORDER BY cursor keeps source column types
- #148 fix(adi): tag ordinals follow creation order
- #152 fix(adt): non-.adt extension (.DAT) works end to end
- #151 fix(session): create honours absolute path with existing parent
  (+ #include <vector> so the new unit test builds under clang)
- #154 fix(engine): partial SEEK keeps Found() with SET DELETED ON
- #155 fix(engine): ordScope bound shorter than key is a prefix
- #153 perf(engine): live key count without per-row goto + recno order

Also includes the CMakeLists resolution that keeps every new regression
case in the unit-test target.
@FiveTechSoft

Copy link
Copy Markdown
Owner

Integrated into main as part of the PR stack merge (9e7ac41). Cherry-picked onto post-CI-fix main, conflict-resolved (CHANGELOG / CMakeLists), and verified locally. Thank you @russimicro.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants