perf: トップ/アイテム詳細/検索一覧の表示速度改善 (issues61802)#1881
Open
mhaya wants to merge 31 commits into
Open
Conversation
Top page, item landing page and search-result page each called get_search_setting() three times per request to read display_control sub-settings (display_facet_search / display_index_tree / display_community). get_search_setting() issues DB queries via SearchManagement.get() and is not cached, so the same setting object was fetched 3x per view (~4 extra DB round-trips each). Fetch display_control once per view and reuse it. Semantics are unchanged (read-only reuse of the same dict); ~12 redundant queries removed across the three pages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three independent issues prevented these tests from running:
- records-ui itemtypes fixture: ItemType uses SQLAlchemy-Continuum
versioning, under which a single interleaved flush could emit the
item_type_mapping INSERTs ahead of their parent item_type rows,
raising fk_item_type_mapping_item_type_id_item_type. Add item_type
rows and flush them before adding the mappings.
- weko-theme db fixture: create_all() creates only the parent
(range-partitioned) user_activity_logs table, not a child partition,
so activity logging during tests failed with "no partition of
relation ... found". Create the current-month partition after
create_all().
- weko-theme test_get_weko_contents: passed a bare string 'comm1' where
get_weko_contents expects request.args (dict-like); 'c' in 'comm1' is
a substring match, so get_community_id then called str.get() and
raised AttributeError. Pass {'c': 'comm1'} instead.
Verified: weko-theme test_utils/test_views, weko-search-ui test_search,
weko-records-ui test_default_view_method[1-4] all pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
default_view_method looped over record.navi paths twice and called Indexes.get_index() for every path in each loop (building path_name_dict and then belonging_community), issuing the same DB lookups twice over. Cache Indexes.get_index() results by path and reuse them in both loops. Behaviour is unchanged; redundant per-path index queries are eliminated. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WidgetDesignSetting.select_by_repository_id() is queried several times while rendering a page (get_design_layout and has_widget_design both read the same repository's design), each time hitting the DB and re-parsing settings JSON. Memoize the lookup on flask.g so it runs once per request. g is cleared each request, so no cross-request invalidation is needed; the result is only read by callers. update()/create() drop the cached entry to avoid serving a stale value within the same request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
get_search_detail_keyword() rebuilt the detail-search conditions on every page render (top page and item landing page): loading all item types and walking the browsing tree each time. The result depends only on the current language and authentication state (guest vs authenticated get different browsing trees); item types / indexes / settings change infrequently. Cache the JSON result on the shared cache keyed by host + language + auth state, with a short TTL (WEKO_SEARCH_DETAIL_KEYWORD_CACHE_TTL, default 300s) so staleness is bounded without needing strict invalidation hooks. The str_ argument is unused by the body, so it does not affect the result or key. test_get_search_detail_keyword clears the cache between calls that change underlying data, since it verifies the computation rather than caching. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two test-fixture gaps in weko-items-ui:
- db fixture: create_all() only creates the parent (range-partitioned)
user_activity_logs table, so activity logging during a test failed with
"no partition of relation ... found". Create the current-month
partition after create_all() (same fix already applied to weko-theme).
- app fixture configured CACHE_* but never called InvenioCache(app_), so
current_cache raised KeyError('invenio-cache'). Initialise it so cache-
backed code paths can be exercised.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The top-page ranking runs up to five Elasticsearch aggregations and then a per-item permission check for every candidate, on every render when the front page is set to show rankings. Cache the result on the shared cache with a short TTL (WEKO_ITEMS_UI_RANKING_CACHE_TTL, default 300s). The key includes the settings signature, the date, and the user identity: guests share one key (high hit rate for the common anonymous case) while each authenticated user gets their own, because permission filtering can surface a user's own not-yet-public items and must not leak across users. Caching is skipped when the cache extension is not configured. This also bounds the per-item N+1 record lookups to once per TTL per user-context. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (4-3) For each search-result hit, sort_meta_data_by_options re-fetched and recomputed the item-type derived data (ItemTypes.get_by_id, option/order lists, hide list and JPCOAR mapping) even though a results page renders many hits that share the same item type. This data depends only on item_type_id and is read-only for the rest of the function (to_orderdict only reads the order list; solst_dict_array is built into a fresh list), so memoize it per request on flask.g. g is cleared each request, so no cross-request invalidation is needed. Verified with test_sort_meta_data_by_options[*] and the sample / no_item_type_id / exception variants. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
default_view_method rebuilt the full JPCOAR OAI-PMH XML on every render (etree.tostring(getrecord(...))) only to feed the Google Scholar / Dataset meta tags. The XML depends only on the record, not the user. Cache it on the shared cache keyed by OAI id + record revision, so an edit (which bumps the revision) invalidates the entry immediately, with a short TTL (WEKO_RECORDS_UI_GOOGLE_XML_CACHE_TTL, default 300s) as a backstop for changes not reflected in the revision. Caching is skipped when the cache extension is not configured. Verified with test_default_view_method (records-ui test app has the cache extension, so the cache path is exercised). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The records_rest item type fixture added item_type_name, item_type and the FK-referencing item_type_mapping in a single begin_nested() flush. ItemType uses SQLAlchemy-Continuum versioning, under which that interleaved flush can emit the item_type_mapping INSERT ahead of its parent item_type row and raise fk_item_type_mapping_item_type_id_item_type. Flush the item type before adding the mapping (same fix already applied in weko-records-ui). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(4-1) Synchronous optimisation of the search-result formatting path (chosen over real threading, which would clash with per-request flask.g memoization and DB-session thread-safety): - sort_meta_data_by_options: the value-matching step scanned the whole solst_dict_array for every metadata item of every field (O(n^2) per hit). Index solst_dict_array by key once and look entries up in O(1). - invenio-records-rest response serializer: remove the ThreadPoolExecutor that was instantiated but never used to submit any work (and its import). The hits are still formatted sequentially on the event loop; behaviour is unchanged, the misleading "parallel" wrapper is gone. Verified with test_sort_meta_data_by_options[*] and test_serializer_response (test_search_responsify). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Record of the top-page / item-landing-page / search-result slowness investigation, written for readers unfamiliar with WEKO3, plus the implementation status mapping each issue to its commit. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- spec.md: functional specification of each performance fix at the function level (target functions, before/after, cache keys/TTL/config, invalidation, risk notes). - unittest_result.md: unit test results per fix, with environment and verification method. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds test/perf_measure/ used to measure top page, item landing page and search-result list response time before/after fix/issues61802: - seed_records.py : clone an existing record into ~3000 dummy records (DB + ES) - measure.sh : curl timing (login + 3 pages) -> results/<label>.txt - browser/measure_browser.mjs : headless-Chromium (Playwright) timing incl. JS + AJAX, external hosts blocked - run_all.sh : switch code via git checkout + uwsgi reload and measure both before and after - nginx-override.yml : remap nginx to 18080/18443 (avoids kind/k8s 80/443) - RUNBOOK.md / pref_result.md : procedure and n=100 before/after results - results/*.txt : raw curl and browser measurements Search list ~2.0x faster (server) / ~1.28x (browser); top page ~1.23x (browser). node_modules and logs are gitignored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Per-iteration probing (30x each) shows the larger after max on the browser detail/search measurements is a single spike (1 outlier ~11s among 29 steady 1.2-1.6s runs), and the same spikes occur on before too (6-7s outliers). Evaluate with median/p90, not max; those are all improved or equal for after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ndence Re-ran the n=100 curl + headless-Chromium before/after E2E after stopping the co-located kind/k8s cluster and killing an unrelated CPU-bound runaway process. Key finding: the speed-up depends strongly on host CPU headroom. - High-load env (CPU saturated): search list ~2.0x, top page ~1.2-1.3x faster. - Clean env (CPU idle): before ~= after on every page (no regression). This matches the nature of the fixes (fewer redundant DB/ES calls + less CPU work): the savings show up as reduced wait time only when the CPU is contended, i.e. under real production load / multi-tenant contention. Clean run is the primary result; the earlier high-load run is kept under results/with_k8s_noise/. max spikes (6-16s) persist in the clean env too, so they are structural noise (worker recycle / GC / ES); evaluate via median/p90. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sequential timing (curl/browser) shows no difference on an idle host because the per-request CPU/DB savings only surface as wait time when server capacity is contended. Added a concurrent load test against the search REST API that reproduces the production "busy site" condition (uwsgi processes=2 x threads=2 =~4 concurrent; drive it past that). Results (400 requests each): - throughput before 0.66 rps -> after 1.43 rps (~2.2x) at all concurrencies - p50 latency before ~2x after (c16: 23.7s -> 11.0s) - total wall before 603s -> after 280s (2.15x) loadtest/loadtest.mjs + run_load.sh; raw data in results/load_*.txt. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Consolidate the sequential (curl/browser) and concurrent load-test results into a single summary table at the top of pref_result.md: no regression when idle, ~2.2x search throughput / ~half p50 latency under concurrency. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…owser.txt
The sequential browser metric showed no before/after difference on an idle
host. Added a parallel-load browser measurement (measure_browser_parallel.mjs:
C=6 concurrent page loads, 48 navigations/URL) that contends the uwsgi tier,
and repointed results/{before,after}_browser.txt to it.
Parallel-load medians: top 3.44s->2.72s (1.26x), search 3.21s->2.15s (1.49x),
detail ~same; throughput top 1.14->1.50, search 1.51->2.06 rps. Sequential
(no-regression) browser data preserved under results/sequential_browser/.
run_parallel_browser.sh writes to a scratch dir and verifies the code marker
before each label, so writing results can't block the before/after git
checkout (which had earlier caused before to be measured on after's code).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…arg) run_load.sh now writes to a scratch dir and verifies the code marker before each label, and accepts the concurrency list as args, so re-running it cannot leave dirty tracked files that block the before/after git checkout. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Checking out the before commit (3de23b2) removes the measurement scripts (they were added later), which broke the before run. Point run_load.sh and run_parallel_browser.sh at /tmp/weko_perf_scripts/ so the scripts persist across the before/after git checkout. The runners themselves are copied to the stable path before running. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-measured at concurrency 20 / 100 iterations: - curl search API: throughput 0.63 -> 1.41 rps (2.24x), p50 29.4s -> 13.8s, 100-req wall 158s -> 71s (2.23x) - parallel browser (20 concurrent page loads): top 16.4s -> 12.7s (1.29x), search 9.3s -> 6.4s (1.45x), detail ~same Runners now switch code with `git checkout <ref> -- modules/` (code only) and verify the marker per label, so test/perf_measure/ (scripts + results) is never touched by the switch and before is always measured on before's code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Summarize the sequential/parallel-browser/concurrent-load results (incl. the concurrency=20 n=100 run) into one cross-cutting table and conclusion: no regression when idle, ~2.2x search throughput and ~half p50 latency under concurrency; evaluate via median/p90/throughput, not max. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Multi-reviewer review flagged three must-fix issues: - [High] common-B get_search_detail_keyword: the "iid" index checkboxes are pruned per user by reduce_index_by_role(roles, groups), but the cache key only had host+lang+auth, so authenticated users shared one bucket and could leak/lose each other's index visibility. Key now includes the admin flag + sorted role ids + group ids (guests share one 'guest' key). - [High] common-B: an admin's SearchManagement change was not reflected for up to the TTL. The key now includes a signature of search_conditions (SM row is fetched once and reused for the body), so a settings change invalidates it. - [Medium] 2-1 get_ranking: ranking titles are language-dependent (switching_language), but lang was missing from the key. Added current language to the key. Tests: add test_get_search_detail_keyword_cache (HIT, settings invalidation, per-user isolation) and test_get_ranking_cache (HIT, per-user + per-language key isolation). Existing detail-keyword / ranking / search / top-page tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the tests the review found missing, each exercising the actual new code path (not just calling the function): - common-A: test_get_weko_contents_fetches_search_setting_once — spies get_search_setting and asserts it is called once per get_weko_contents. - common-C: test_select_by_repository_id_request_cache — per-request memoization HIT (2nd read does not touch the DB) and update()/create() invalidation. - 4-3: test_sort_meta_data_by_options_memoization — item-type data is fetched once for two hits of the same item type in one request, and the two hits produce identical output (cache treated read-only). - 3-1/3-2: test_default_view_method_caches_oai_xml_and_index — getrecord not re-run on the 2nd render (OAI XML cache HIT), and Indexes.get_index runs at most once per distinct navi path (index memoization). All pass; existing detail-keyword / ranking / widget update-create / top-page / default_view_method tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-ran the load + parallel-browser measurement after the review fixes (common-B cache key now includes role/group; 2-1 key includes language). Performance is maintained: - curl search API: 0.67 -> 1.43 rps (2.13x), p50 29.1s -> 13.6s, wall 149s -> 70s - parallel browser: top 15.08s -> 11.93s (1.26x), search 12.81s -> 8.74s (1.47x) Matches the pre-review-fix numbers, so closing the index-visibility leak did not regress the speedup. pref_result.md updated (TL;DR, sections ②/③, final table); raw data refreshed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Load test at 40 concurrent clients (100 requests): - curl search API: throughput 0.63 -> 1.40 rps (2.22x), p50 58.0s -> 27.5s (30.5s shorter), wall 159s -> 71s (2.23x) - parallel browser (C=40): top 30.28s -> 23.18s (1.31x), search 22.24s -> 15.27s (1.46x); detail inverts due to client-side (40 chromium) saturation. Throughput stays ~2.2x across concurrency 4..40 (uwsgi ~4-concurrent is the bottleneck), while the absolute p50 latency gap widens with load (15.5s at c20, 30.5s at c40). pref_result.md section ③ updated; raw data in load_*_c40.txt and *_browser_c40.txt (section ② keeps c20 as primary). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two settings getters ran a DB lookup on every call (get_search_setting once per page render; AdminSettings.get from ~70 call sites incl. 3x in the item landing page). Both change only when an admin saves them, so cache the value per key with a short TTL (WEKO_ADMIN_SETTINGS_CACHE_TTL, default 300s): - get_search_setting(): cached under one key; SearchManagement.update()/create() invalidate it via delete_search_setting_cache(). - AdminSettings.get(): cached per name; update()/delete() invalidate the entry. Both store and return deep copies so callers cannot corrupt the shared cache, and skip caching entirely when the cache extension is not configured. These are global (not per-user) settings, so no cross-user leak. (The per-hit pickle deep-copy in sort_meta_data_by_options was considered but left as-is: those two copies snapshot the pre-hide metadata state and are not safe to drop.) Tests: test_get_search_setting_cache and TestAdminSettings::test_get_cache cover cache HIT, copy-on-return (mutation safety) and invalidation; existing get/update/delete and top-page/detail tests still pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extended loadtest.mjs with top/detail targets (new `target` arg) and added
run_load_pages.sh to load-test the pages where the settings-getter caches
(common-A/B/C + B AdminSettings.get + D get_search_setting) apply.
Findings (pref_result.md §④):
- No steady-state regression: warm top page p50 1065ms -> 843ms (1.26x),
p90 1124 -> 1014ms. Detail ~equal.
- But the settings-cache effect is small: top/detail time is dominated by
template rendering, so the few settings queries removed are in the noise;
these pages (14-18 rps) are not the bottleneck (search API at 1.4 rps is).
- Cold-start caveat: right after a cache flush, 20 concurrent requests all
miss and rebuild simultaneously (thundering herd), spiking p90 to ~12s;
gone once warm. Documented as an operational note.
Raw data: results/load_{before,after}_{top,detail}_c20.txt.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…avior
Browser networkidle time is mostly NOT server work: a top-page load pulls 42
static assets (nginx) + JS exec + render; the app server is ~12% of it (curl
HTML-only 0.19s vs single browser load 1.63s). Chromium also saturates the
client CPU past ~4-6 concurrent pages (throughput peaks at conc=4, tail
explodes at conc>=6), which dilutes the before/after server gap at conc=20.
Measured at concurrency=5 with a warm cache (run_parallel_browser_warm.sh,
pre-warm avoids the cold-start stampede): top 3.01s -> 2.17s (1.39x), search
2.36s -> 1.73s (1.37x), detail ~equal. The lower concurrency shows the server
improvement more cleanly than conc=20 (top 1.26x). For a rigorous server-side
before/after, the curl load test (§③) remains the reliable metric.
pref_result.md §② updated with the concurrency sweep and conc=5 results;
raw data in results/{after,before}_browser_c5.txt.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
nginx/Dockerfile is the amd64 build path (docker-compose.yml -> build: ./nginx), but 3de23b2 rewrote its .deb filenames to _arm64.deb, so the COPY --from stage failed on amd64 hosts. dpkg-buildpackage names the artifact after the build host architecture, so match it with a wildcard instead of hard-coding the arch in both Dockerfile and Dockerfile.arm64. Separately, `apt-get update` sat in its own layer ahead of `apt-get install`. Once that layer was cached the index went stale and pointed at package versions the archive had already superseded, e.g. E: Failed to fetch .../distro-info-data_0.43ubuntu1.19_all.deb 404 Fold update into the same RUN as the install/source/build-dep it feeds, in all three Dockerfiles. Verified by a full `docker build -f nginx/Dockerfile` on arm64: the wildcard resolves nginx_1.20.1-1~focal_arm64.deb and distro-info-data now installs at 0.43ubuntu1.20. The amd64 path and Dockerfile.ams are unverified here (arm64 host; ams pagespeed psol is x64-only). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
概要 / Summary
トップページ・アイテム詳細(ランディングページ)・検索結果一覧の表示速度改善。
1リクエスト内で不要に繰り返される DB/ES アクセスと CPU 計算を削減し、混雑時のスループット・レイテンシを改善する。
調査・仕様・計測結果は以下のドキュメントに記録:
pref_issues.md— 課題の調査(該当コード・原因・修正方針・期待効果)spec.md— 各修正の機能仕様(ファンクションレベル)unittest_result.md— ユニットテスト結果test/perf_measure/pref_result.md— E2E 性能計測(before/after)パフォーマンス修正 / Performance fixes
get_search_setting()を各ビューで3回→1回に集約get_search_detail_keyword()を短TTLキャッシュ(host+lang+auth キー)get_ranking()を短TTLキャッシュ(guest共通 / 認証ユーザー別キーで漏洩防止)*_CACHE_TTLconfig で調整可)。キャッシュ拡張未設定の環境ではスキップして素の計算にフォールバック。入出力の意味は不変。
テスト基盤修正 / Test-infra fixes
計測・検証のため既存のテストフィクスチャ不具合も修正(本番コードには影響しない):
user_activity_logsの当月パーティション未作成(weko-theme / weko-items-ui)item_type_mappingの FK 挿入順(SQLAlchemy-Continuum)(weko-records-ui / invenio-records-rest)InvenioCache初期化漏れ(weko-items-ui)、テストの文字列引数バグ(weko-theme)性能計測 / Performance measurement (
test/perf_measure/)docker-compose.arm64.ymlスタックに約3000件のダミーレコードを投入し、curl と headless Chromium でbefore/after を E2E 計測。手順・スクリプト・生データを同梱(
RUNBOOK.md)。結論: 平常時(低負荷)にリグレッションは無く、アクセスが混雑するほど大きく効く。
レイテンシ改善として顕在化する。並行度4〜20でスループット約2.2倍が一貫。
テスト / Testing
各修正について、変更行を通る対象ユニットテストが PASS することを稼働中の docker テスト環境で確認済み
(詳細は
unittest_result.md)。🤖 Generated with Claude Code