test(backlinks): Performance check for backlinks retrieval at scale (now shares production's viewer-filter query) - #11622
Conversation
|
Tick the box to add this pull request to the merge queue (same as
|
Requirement 3.4 asked for evidence that a heavily-linked hub page's backlinks
come back in interactive time. Measured: median 127 ms against 100,001 pages /
205,000 link rows with a 5,000-inbound hub, ~8x under the 1 s target. Raising the
hub to 20,000 inbound costs only ~1.5x (189 ms), so the shape is sub-linear and
the budget is not at risk at any realistic scale.
Both read queries are index-backed with no COLLSCAN: the distinct rides toPage_1,
the viewer filter rides _id_, and the save-path delete filter ({fromPage, toPath:
{$nin}}) rides fromPage_1_toPath_1 — so the two indexes B2.2 kept are sufficient
and no index/query fix falls out of this task. The no-rescan guarantee is checked
directly: one syncOutboundLinks issues exactly one bulkWrite whose every operation
filter is scoped to the edited fromPage, a sibling page's rows are byte-identical
afterwards, and the collection total moves only by that page's delta.
Written as an env-gated integ test rather than a CI test or a standalone script
(the decision B2.1 asked to make first). It is collected by the app-integration
project but skipped unless BACKLINKS_PERF is set, so CI never pays the 5 s seed,
while the measurement stays in-tree and reuses the crowi harness — it calls the
real findBacklinks instead of a hand-rolled copy that could drift. It expects a
real MongoDB via MONGO_URI; the default in-memory server would produce numbers
that say nothing about a deployment. The harness rewrites the db name to
growi_test_<workerId>, so the dev database is never touched, and the seed is
deleted by id in afterAll.
The seed also asserts the result set (4,000 of 5,000 sources visible under a mixed
grant/status distribution), so it doubles as a correctness check at a scale the
other integration tests do not reach.
Recorded in tasks.md with the reproduction command, the environment, and one
observation left deliberately unactioned: the distinct is FETCH <- IXSCAN rather
than a covered DISTINCT_SCAN, because its key (fromPage) is not in the index it
rides. A {toPage, fromPage} compound would cover it, but that is 10% of a read
with 8x headroom and would re-add per-save write cost B2.2 just removed.
Depends on B2.2 (#11611): the index assertions encode the two-index state that
branch introduced.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
53d226e to
5ab263b
Compare
Review of the first version found two ways the seed made the result look better than production would. 1. Pages were bodyless stubs carrying only the fields the read path filters on — 185 B against the 384 B a real wiki averages. The viewer filter is a FETCH plus projection, so WiredTiger reads whole documents and document size is exactly what it pays. Pages now carry the full real field set (revision, creator, lastUpdateUser, parent, descendantCount, latestRevisionBodyLength, seenUsers, commentCount, __v), giving a 427 B average. The reference ids point at nothing; only their bytes matter, so no Revision documents are needed. 2. The viewer belonged to no user groups, so findAllUserGroupIdsRelatedToUser returned [] and generateGrantCondition omitted its GRANT_USER_GROUP branch entirely — the measured query was structurally simpler than any real group member's. The viewer is now in two real UserGroups, and 20% of the hub's sources are group-granted (half to a group the viewer is in, half to one they are not), so the grantedGroups $elemMatch branch is exercised and both outcomes asserted. Neither changed the answer: 2.3x bigger documents plus the extra branch cost ~1 ms (127 -> 128 ms). They were worth fixing because the old seed made the number look optimistic, not because the correction mattered. Also closes the "everything is cache-resident" objection, which no amount of seeding fixes. BACKLINKS_PERF_COLD=1 shrinks the server's WiredTiger cache below the working set, measures, and restores the exact original in a finally. With the cache 19x too small (8 MiB vs 155 MiB) the read is 164 ms — still ~6x under target, so the warm figures are not an artifact of a hot cache. Opt-in and off by default: it mutates a server-wide setting, and a process killed mid-test would leave the cache small until mongod restarts. That hazard is documented at the top of the file. The seed now also reports average document size, working-set size and the cache ceiling, so a future run states which regime it measured instead of leaving the reader to assume. Numbers in tasks.md and the find-backlinks comment updated; the 20,000-inbound column was re-measured against the new seed rather than left mixing datasets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…asserting it B2.1 recorded that findBacklinkSources' distinct is not covered, and justified leaving it that way with "a third index would re-add per-save write cost that B2.2 just removed". That was reasoning, not measurement, and it is wrong. measurements/b21-index-cost.mjs measures all three axes over 205k rows, A/B/A' against drift on a shared box: storage +3.2 MiB (~16 B/row) per-save write (10 links) 6.05 -> 6.04 ms (no measurable cost) distinct, 1 row per src->tgt 7.3 -> 10.1 ms (44% WORSE) distinct, 3 rows per src->tgt 13.3 -> 9.0 ms (39% better) So the index is nearly free and the write-cost argument does not hold. The reason to skip it is the third row: DISTINCT_SCAN earns its keep by skipping duplicate keys, and when each source links a target once, every fromPage under a toPage is already unique — nothing to skip, and a wider index to walk. It only pays when one page links the same target several ways (path + permalink + anchor), which is the case B1.15 asserts is de-duplicated. Conclusion unchanged, rationale replaced, and the open question narrowed to something empirical: do real wikis average >=2 rows per source->target pair? The script is the way to check, so it is committed next to the spec rather than left in a scratch directory. The script documents the two mistakes that produced wrong numbers first time round: reading index sizes across an intervening write phase (which attributed growth in the existing indexes to the new one, reporting "+140%"), and trusting a single before/after pair on a box whose timings drift. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…backlinks-read-perf
`PageLink.init()` only ever creates indexes, and the harness reuses
`growi_test_<workerId>` across runs without ever dropping it. A `pagelinks`
built before B2.2 removed the `{fromPage}` / `{toPath}` declarations therefore
keeps those indexes, and the inventory assertion reports a stale local database
as a real gap rather than a genuine one.
`syncIndexes()` drops what the schema no longer declares and creates what it
does, so the benchmark measures the shipped index set. Verified from a broken
starting state (`_id_, fromPage_1, toPath_1`): it converges to
`_id_, toPage_1, fromPage_1_toPath_1` and the benchmark passes, where the same
state previously failed on COLLSCAN.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
コードレビュー: PR #11622タイトル: test: Performance check for backlinks retrieval at scale 全体評価本番コードの変更は 指摘事項(重要度順)1. [再現性] 計測スクリプトのモジュール解決パスがハードコードされている
index 追加を見送るという判断の唯一の根拠がこのスクリプトの計測結果であるため、その根拠が誰にも再現できない状態になっている。 対応: 2. [運用] 再現コマンドが共有 mongod を 64 MiB キャッシュに縮退させうる場所: tasks.md:266 記載された再現コマンドが restore 前にプロセスが kill されると、mongod 全体が 64 MiB キャッシュのまま残り、以降の開発作業すべてが遅くなる。 対応: 再現コマンドから 3. [正当性] cleanup が想定外の行を削除しうる場所: page-link-read-perf.integ.ts:415
到達経路は実在する: 前回の kill された run で 対応: 4. [テスト設計] 「exactly the two shipped indexes」を名乗りながら index 件数を assert していない場所: page-link-read-perf.integ.ts:433 コメントは「出荷済みの2本の index のみが存在する状態で測る」と宣言しているが、実際には index 件数を検証していない。 このため、まさに本タスクが却下した 対応: 5. [テスト設計] env var の typo がグリーンな偽陽性になる場所: page-link-read-perf.integ.ts:48
対応: 6. [正当性] キャッシュサイズの復元がコメントの主張と食い違う場所: page-link-read-perf.integ.ts:518 コメントは「exact original, not a rounded guess」と書いているが、実際には 対応: コメントを実態に合わせるか、元が自動サイズだった場合は復元しない分岐を入れる。 7. [重複] viewer フィルタのクエリ構築が本番コードから複製されている場所: page-link-read-perf.integ.ts:466 viewer フィルタのクエリ構築が find-backlinks.ts から2箇所コピーされており、drift 防止手段はコメントによる注意喚起のみ。 本番ロジックが変わってもベンチマークは古いクエリを測り続けるため、数字が実態から乖離する。 対応: 可能な範囲でクエリ構築を本番側から import して共有する。 問題なしと確認した点spec と実装の一致
正当性の検証
dead code見つからなかった( セキュリティ
テスト設計(essential-test-design / essential-test-patterns)
|
…m its own path
createRequire was anchored at a hardcoded /workspace/growi/apps/app/, so the script
died with "Cannot find module 'mongodb'" on any other checkout — and the B2.1 decision
to skip the { toPage, fromPage } compound index rests on its numbers, so that evidence
was not reproducible by the reviewer.
Anchor it at import.meta.url instead. createRequire still points into apps/app because
mongodb lives only in that workspace's node_modules (pnpm does not hoist it to the repo
root), so a plain import from .kiro/ cannot resolve it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…mongod The recorded reproduce command pointed BACKLINKS_PERF_COLD=1 at mongodb://mongo:27017 — the shared devcontainer instance — directly contradicting the test file's own "never point it at anything but a throwaway MongoDB". The growi_test_<workerId> rewrite quoted one sentence earlier reads as covering it, but does not: the cold run shrinks the cache via setParameter wiredTigerEngineRuntimeConfig, which is process-wide, and its restore is a finally that survives a failed assertion but not a killed process. Keep the warm command as-is (it only touches its own database) and document the cold run separately: why the shared instance is off limits, and a host-started throwaway container whose disposal *is* the restore. The devcontainer has no docker CLI and mongo is a sibling compose service, so the container is started from the host and reached by name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… B2.1 benchmark
Four review findings, all in the benchmark itself:
- Cleanup could delete rows it never created. A beforeAll that throws before hubPageId
is assigned still runs afterAll, and mongoose passes {toPage: undefined} through to
the driver, which serializes to {toPage: null} — matching every unresolved link row in
the database (toPage defaults to null). Verified on a probe: 2 of 3 docs deleted, the
resolved one surviving. Guard the delete, and route the fixed-name cleanup through a
purge shared with beforeAll so a killed run self-heals instead of poisoning the next
one on the unique username / UserGroup name indexes.
- "Exactly the two shipped indexes" asserted presence, not the set, so the very
{ toPage, fromPage } compound this task rejected could be added with every test still
green — the distinct merely upgrades to PROJECTION_COVERED <- DISTINCT_SCAN, still
index-backed and still under target, while the recorded numbers silently describe a
configuration that no longer exists. Assert the closed name set instead.
- A malformed scale value made the measurement pass vacuously. Number('10_000') is NaN
(the file's own style invites that typo) and Number('') is 0, which ?? does not
intercept; both make every seeding loop run zero times, and the read-path assertion
then compares 0 against an expectedVisibleCount the same broken loop computed as 0.
Validate up front — in beforeAll, not at module scope, so a stale export cannot break
collection for the whole app-integration project.
- The cold run only *reported* its cache restore, under a comment claiming an exactness
the Math.round'd MiB value cannot provide. Assert the restore in bytes (lossless:
mongod sizes its cache in whole MB), from outside the finally so a failing body is not
masked, plus a warning from inside the finally for the case the assertion cannot reach.
The index and scale guards were mutation-checked: adding { toPage, fromPage } to the
schema fails only the inventory test (the other three stay green, as described above),
and each bad scale value now fails naming its own cause.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The B2.1 benchmark reproduced find-backlinks.ts' query construction twice — once to time
the viewer-filter sub-step, once to explain it — guarded only by a comment asking future
editors to keep them in step. So the two claims that rest on those copies were not claims
about production: the "~90% of the total is the viewer filter" attribution recorded in
tasks.md, and the no-COLLSCAN guarantee. A production-side change (a cap, a sort, a
grant-filter restructure) would have left both green while measuring a query that no
longer existed.
Extract buildVisibleSourcesQuery and have findBacklinks and the benchmark share it.
Behaviour is unchanged: same builder calls in the same order, same projection, same
execution — only the callers now decide whether to run it or explain it.
It returns { query } rather than the query itself because a mongoose Query is thenable:
await on a Promise<Query> chains into it and resolves to the executed rows, which would
defeat handing back an unexecuted query. The wrapper is not thenable, so it survives the
await. The typed boundary also means explain() is no longer implicitly any (mongoose
types it as resolving to the query's own result type), hence the annotation at that call.
Mutation-checked: .hint({ $natural: 1 }) in the production helper turns the benchmark's
reported plan into PROJECTION_SIMPLE <- COLLSCAN and fails it. The old copy-based version
stayed green and kept reporting FETCH <- IXSCAN.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
https://redmine.weseek.co.jp/issues/185820
https://redmine.weseek.co.jp/issues/187968
Important
This is a stacked PR. It targets
feat/187679-coalesce-link-extraction, and#11611 must be merged first.
Task B2.1: prove requirement 3.4 — a heavily-linked page's backlinks return in interactive time — and find the bottleneck while an index fix would still be cheap.
Almost entirely a benchmark, the recorded result, and one stale comment updated. One production change came out of review:
find-backlinks.tsnow exposes its viewer-filter query so the benchmark measures the query production actually issues rather than a copy of it — behaviour-preserving, isolated in one commit. See Review follow-up at the end.Result: 128 ms against a 1 s target
100,001 pages / 205,000 link rows, hub page with 5,000 inbound sources:
findBacklinks(full read path)findBacklinkSources(distinct)Pagequerydistinct— a_id: {$in: [5k]}fetch plus the grant$or, so it scales with source count as expected.bulkWrite, every filter scoped to the edited page.Environment and measurement caveats
Devcontainer MongoDB 8.2.9, wiredTiger,
rs0single-node replica set, 16 cores / 15.9 GB. Medians of 5 runs after a discarded warm-up. Repeated runs land at 116–130 ms median; the max is the noisy figure since the devcontainer shares CPU. Read these as an order of magnitude under target, not a number to regression-test against.Plans in full:
distinct→FETCH <- IXSCANontoPage_1; viewer filter →PROJECTION_SIMPLE <- FETCH <- IXSCANon_id_; save-path delete →FETCH <- IXSCANonfromPage_1_toPath_1.Cold-cache runs (
BACKLINKS_PERF_COLD=1, which shrinks the server's WT cache below the working set and restores it afterwards):Realism of the seed: pages carry the full real field set, averaging 427 B against the 384 B a real wiki shows — document size is what a FETCH-and-project read pays. The viewer is a member of two real
UserGroups, sogenerateGrantConditionemits itsgrantedGroups: {$elemMatch: …}branch; without groups seeded that branch is omitted and the query measured would be simpler than a real member's. Both were corrected after review, and together they moved the result by ~1 ms.How it works
One gated file: build a world once → ask five questions of it → remove it.
They are ordered premise → result → explanation → adjacent guarantee:
findBacklinksas a viewer, and each half separately, so the output says which half owns the time.COLLSCANwill betray you at the next data size.explain()turns "128 ms today" into "128 ms because both queries ride an index".Two properties worth knowing:
Why gated rather than in CI
B2.1 asked for this decision up front. It's an env-gated integ test — collected by the
app-integrationproject, skipped unlessBACKLINKS_PERFis set:MONGO_URI=mongodb://mongo:27017/growi?replicaSet=rs0 \ BACKLINKS_PERF=1 pnpm vitest run page-link-read-perffindBacklinksthrough the crowi harness; a hand-rolled copy of the read path could drift and quietly measure the wrong thing.growi_test_<workerId>, and the seed is deleted by id inafterAll(verified empty afterwards).The one index question, measured
Step one of the read asks the
pagelinkscollection: which pages link to page X? It uses the index ontoPage, so it finds the ~5,000 relevant rows instantly. But an index entry only stores the field it was built on — it doesn't carry the source page id, which is exactly what we asked for. So Mongo opens all 5,000 rows just to read one field out of each. That's theFETCHinFETCH <- IXSCAN.Indexing both fields (
{toPage, fromPage}) would put the answer entirely inside the index, with no rows to open — a "covered" query, which Mongo reports asDISTINCT_SCAN. Worth adding?Measured with
.kiro/specs/backlinks/measurements/b21-index-cost.mjsover 205k rows. Every figure is taken as baseline → with the index → baseline again, because this machine's timings drift enough that a single before/after pair proves nothing:Those last two rows are the whole story, so to be explicit about what varies: a source page usually produces one link row per target it points at. It produces several when it links the same target more than one way — by path, by permalink, and by anchor all resolve to the same page, and each is its own row.
Answer: don't add it — not because it's expensive, but because it makes the normal case slower. A covered
DISTINCT_SCANearns its keep by skipping over duplicate index entries. When a source links a target exactly once, there are no duplicates to skip, so all it does is walk a wider index — hence 44% worse. Only when sources link the same target repeatedly does the skipping pay off.That leaves a narrower, factual question I can't answer from a synthetic dataset: in real wikis, does a page typically link the same target more than once? If it commonly produces two or three rows per target, this decision flips, and the committed script is how to check.
Two side notes: this index couldn't be reused for the
toPathlookup a later task needs (wrong field order), so skipping it now doesn't create work later. And the de-duplication itself is already covered by tests — a source that links a target twice is listed once in the backlinks result.Verification
lint:typecheckclean ·vitest run src/features/backlinks→ 104 passed, 5 skipped (gate working) · full-scale benchmark 5/5, WT cache confirmed restored afterwardsRe-verified after the review follow-up:
lint:biomeclean ·lint:typecheckclean for the touched files ·vitest run src/features/backlinks→ 111 passed, 5 skipped (gate still working) · gated benchmark 4 passed / 1 skipped, plan back toPROJECTION_SIMPLE <- FETCH <- IXSCAN. The follow-up runs were at reduced scale (BACKLINKS_PERF_PAGES=800), since each fix was checked by mutation rather than by re-measuring; the recorded 100k figures above are unchanged and were not re-run.Review follow-up
All seven findings from @miya's review addressed:
cd71bad·e85ac3c·56b457f·35306f2.Warning
Scope change: this PR now touches production code. The review correctly noted the only
production change was a comment in
find-backlinks.ts. Finding #7 required extracting theviewer-filter query construction so the benchmark measures production's own query instead of a
duplicate. Behaviour is unchanged — same builder calls, same order, same projection — and it is
isolated in
35306f2(revertable without touching the other six), but it is a production diffand wants re-review as such.
import.meta.urlBACKLINKS_PERF_COLD=1no longer suggested against the shared instance{toPage: undefined}→{toPage: null}delete; fixed-name cleanup now self-heals after a killed runb21-group-a→ RED without the purge (E11000 dup key). A foreign unresolved-link row was confirmed to survive a full runpagelinksindex inventory{toPage, fromPage}to the schema fails only this test — the plan and latency tests stay green, which is exactly the holebeforeAll(not module scope, so a staleexportcannot break collection for the whole project)10_000,"",5k, andINBOUND > PAGES— each now names its own cause7790919680= exactly 7430 MiB)buildVisibleSourcesQueryshared byfindBacklinksand the benchmark.hint({$natural: 1})in production turns the reported plan intoPROJECTION_SIMPLE <- COLLSCANand fails the test; the old copy-based version stayed greenBeyond what the review asked, all inside this PR's own benchmark: the self-heal purge (#3 asked only for the guard), the
INBOUND > PAGEScheck (#5), and asserting the cold restore rather than just fixing its comment (#6).Deliberately not changed:
collectStages(test) andplanStages(b21-index-cost.mjs) stay duplicated. The standalone script cannot import TypeScript fromapps/app/srcwithout a build step, and unlike the query copies, nothing misreports if the two drift — each explains its own query.One loose end: on a combined run of
backlinks.spec+page-link-lifecycle.integ+page-link-service.integ,create adds a backlink, and a later update removes itfailed once, then passed on re-run and in isolation (6/6). It looks like a queue-drain timing flake unrelated to these changes — every viewer-visibility test in that file passes consistently, and a broken query would fail them deterministically — but I did not confirm it against a pre-change baseline.