Skip to content

perf: mods read one shared copy of each list and of keys() - #545

Open
abhijit-s wants to merge 5 commits into
herdrdev:masterfrom
abhijit-s:perf/shared-mod-reads
Open

abhijit-s wants to merge 5 commits into
herdrdev:masterfrom
abhijit-s:perf/shared-mod-reads

Conversation

@abhijit-s

Copy link
Copy Markdown
Contributor

TL;DR: panes(), agents(), terminals() and the other snapshot reads hand the script engine one shared copy per revision instead of converting the whole list on every call, and keys() is listed again only when keys.json or the mods folder changes. Mod script time per frame roughly halves in a 30-agent layout.

Why

What changes

  • gpui-kit: HostValue::Json(Arc<serde_json::Value>), host-to-script only. The QuickJS bridge builds the script's copy straight from the shared value (json_into_js). Each call still returns a fresh JS object, so a mod that mutates what it read sees no difference (test a_shared_json_value_reaches_the_script_as_its_own_copy).
  • src/mods.rs: reader serves shared_part(key), copied once per revision of that key (the revisions upstream already keeps to redraw only affected mods). keys() serves keys_part(), rebuilt when keys.json or the mods folder's modified time changes.
  • defaults/mods/tabs/main.js: reads panes() once per render.
  • defaults/AGENTS.md: one line telling mods each list read is a fresh copy, so read once per render.

Numbers

Measured on my fork, whose mod read path matched upstream's apart from this change, with a headless app, a 30-agent layout (5 spaces × 6 tabs) and fake agents switching working/idle; two runs per side, p50 values:

Before After
One panes() read, 30 panes 58 µs 45 µs
One terminals() read, 30 terminals 168 µs 122 µs
One keys() read 355 µs ~125 µs
Script time per mod frame, idle 12.9 ms 6.8 ms
Script time per mod frame, status storm 14.6 ms 7.8 ms
Mod frame, idle 21.6 ms 16.6 ms
Mod frame, status storm 25.8 ms 18.9 ms
App instructions, idle / storm (M/s) 166 / 740 137 / 603
  • A cached JSON text handed to JSON.parse was tried first and was slower than building the objects (QuickJS's JSON parser).
  • Most of the remaining mod frame is GPUI layout, prepaint and materialize of the panels' trees.

Tests

  • mods::tests::reads_share_one_copy_of_a_snapshot_part_until_it_changes, mods::tests::keys_are_listed_again_only_when_their_files_change, and the gpui-kit bridge test above.
  • just check passes.

@abhijit-s

Copy link
Copy Markdown
Contributor Author

TL;DR: the gpui-kit test this PR adds, a_shared_json_value_reaches_the_script_as_its_own_copy, passes. just check doesn't run gpui-kit's tests, so I ran them separately.

  • How: cargo test -p gpui-shell --lib in vendor/gpui-kit, with that workspace pointed at the root's [patch] entries and Cargo.lock. Standalone, it resolves a newer rsa that llrt_crypto doesn't compile against, and stock gpui-pre without spin_clock_ms.
  • Result on this branch: 717 passed, 5 failed.
  • Result on master (e77f112): 716 passed, the same 5 failed, so they come from upstream, not this PR:
    • plugin::tests::runtime_check_returns_the_rendered_description
    • tests::render::a_readonly_script_editor_remains_selectable
    • tests::render::the_bundled_example_application_loads_and_renders
    • tests::render::the_todolist_example_exercises_the_runtime
    • typings::tests::every_element_method_is_accounted_for

@abhijit-s

Copy link
Copy Markdown
Contributor Author

TL;DR: merged master, including 029323e (mod code reused across copies).

  • Conflict: only src/mods.rs, where this PR's two tests and 029323e's loading_copies_reclaims_closed_command_channels_without_dispatch were added at the same place; all three are kept.
  • host_modules.rs: 029323e's rewrite and this PR's HostValue::Json / json_into_js merged without conflict; the PR's diff against master there is still only those additions and their test.
  • just check passes locally on macOS. I haven't re-run the gpui-kit suite (cargo test -p gpui-shell --lib) on the merge; the numbers above are from before it.

…eads

# Conflicts:
#	defaults/mods/tabs/main.js
This branch's reader hands mods one shared copy of panes() per revision, which skips read(), where upstream's herdrdev#1015 fills in each terminal pane's scroll. panes_part lays the current scroll numbers over the shared copy and reuses the result while the copy and the numbers are unchanged, so panes().scroll stays live, as herdrdev#1015 documents, and reads still share one copy.

This branch has not been deployed

No deployments
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