Skip to content

perf: one access seam for InternalValue outside wave 2's files (0140 P0a) - #453

Merged
rmorozov merged 4 commits into
masterfrom
claude/project-thread-o9wo6j
Oct 11, 2026
Merged

rmorozov merged 4 commits into
masterfrom
claude/project-thread-o9wo6j

Conversation

@rmorozov

@rmorozov rmorozov commented Oct 10, 2026 •

Copy link
Copy Markdown
Member

Requested by Ruslan · project thread

Before: code all over src/ read InternalValue through how it is stored: GetIf<ListAdapter>, GetData(), std::get_if on the variant, switches on its index. P2 (the 32 B layout in docs/design/0140-internal-value-plan.md) would have had to edit every one of those sites.

After: outside internal_value.* and wave 2's files, values are read through one seam that P2 can keep. Behaviour is unchanged except for one parity fix: reverse of a dict.items() key now gives a string, as in Python. Before, it gave a list of characters.

The seam, all in src/internal_value.h:

  • ValueKind / InternalValue::Kind() says what a value is.
  • AsList / AsMap return non-owning ListRef / MapRef views. Overloads taking a temporary are deleted.
    • Visitors now receive the views, not the adapters. They get them as const lvalues, so that a const ListRef& overload beats a forwarding-reference template.
    • ToAdapter() makes the owning copy where a site keeps the list.
  • AsStringView<CharT> and TakeString read and take strings. IsStringValue moved here from markup.h and value_methods.cpp.
  • IMapAccessor::SetValue is const, so a value can hold its accessor as a const object.

Left for P0b, once wave 2 releases its files: the callers of GetIf<ListAdapter>/GetIf<MapAdapter>, GetStringView and NarrowStringView in expression_evaluator.* and statements.*, then GetData() itself.

How. Each site was converted mechanically, then recounted under Callgrind. The first count was +0.2..+6.6% Render, so three hot paths were reworked:

  • IsNumber keeps per-alternative tests. A Kind() comparison cost +6.6% on expressions.
  • OutStream::WriteValueTo tests int, string, host value and bool in turn. Strings meet at one WriteString, which GCC then keeps inline.
  • ConvertToList returns a list's copy in place. Before, it copied twice, through std::optional.

Instruction counts (bench/count.py, Release, vs ec9e7b4; allocations and memory identical on every case):

case change
Render/mitsuhiko_table -1.56%
Render/mitsuhiko_table_wide -1.76%
Render/filters -0.88%
Render/for_range -0.61%
Render/expressions -0.46%
Render/large_static +0.55%
Render/chat_qwen +0.46%
Render/chat_llama +0.36%
every other Render and Load case within ±0.3%

Render/large_static's +0.55% is +150 instructions per render, about 2 per string write in WriteValueTo. It is an interim cost of the seam. P2, the 32 B layout, recovers it, together with #445's Load/plain_text +0.75%; the 0140 task file records both.

Parity. New corpus case filters.reverse_dict_item_key pins the reverse fix. filters.dictsort_pair_subscript pins a gap found on the way: p[0] of a dictsort pair is undefined. It is filed as docs/tasks/0159 and listed in divergences/filters.txt.

Process. Two verifier rounds ran.

  • Round 1 found two clang-tidy errors (use-after-move in ParseAdjacentStrings, a misplaced NOLINT), both fixed.
  • Round 2 found an unused include, removed.
  • Both rounds also ran Debug, ASan+UBSan, a Clang 18 C++20 build and a 300-template differential against master and Python. The reverse fix was the only difference.

Red pushes so far: 0. scripts/preflight.sh passes.

🤖 Generated with Claude Code

https://claude.ai/code/session_014gsczz1ZU7Lu24hymWrNJv

InternalValue's layout is about to change (P2: 32 B, trivially copyable
immediate plus one intrusive pointer). Code outside internal_value.* now
reads values through an interface that layout can keep:

- ValueKind and InternalValue::Kind() say what a value is.
- AsList/AsMap return non-owning ListRef/MapRef views; visitors receive
  the views (as const lvalues, so their overloads beat forwarding-
  reference templates) instead of the adapters. ToAdapter() makes the
  owning copy where a site keeps the list.
- AsStringView<CharT> and TakeString read and take strings;
  IsStringValue moves next to them from markup.h and value_methods.cpp.
- IMapAccessor::SetValue is const, so a value can hold its accessor as a
  const object.

Instruction counts stay within 0.5% of master except Render/large_static
(+0.55%, 150 instructions); mitsuhiko_table is 1.6% lower because
ConvertToList now copies a list once instead of twice. Allocations are
unchanged.

reverse() of a plain std::string value (a key of dict.items()) now gives
a string, as in Python, instead of a list of characters. The dictsort
pair subscript gap found on the way is filed as 0159.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014gsczz1ZU7Lu24hymWrNJv
@rmorozov rmorozov self-assigned this Oct 10, 2026
@rmorozov
rmorozov marked this pull request as ready for review October 11, 2026 08:06
@rmorozov
rmorozov merged commit 66c143e into master Oct 11, 2026
41 checks passed
rmorozov added a commit that referenced this pull request Oct 11, 2026
0147, 0152 and 0154 are done; 0158, 0159 and 0160 are new.


Claude-Session: https://claude.ai/code/session_01KMcWcpGwhiaEs33YjAgEV4

Co-authored-by: Claude <noreply@anthropic.com>
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