Skip to content

Move constant values of map literals into the template - #1206

Closed
luispedro wants to merge 1 commit into
rhaiscript:mainfrom
luispedro:map-template-constants
Closed

luispedro wants to merge 1 commit into
rhaiscript:mainfrom
luispedro:map-template-constants

Conversation

@luispedro

Copy link
Copy Markdown
Contributor

The second item from #1196: the constant values of a map literal go into its template, so only the computed values are evaluated each time the literal is. This is separate from #1202 (the constant pool index), as suggested there.

Changes

  • Optimizer: after optimizing a map literal's entries, it moves the constant values into the template (Expr::lift_map_constants). If that leaves no entries, the next pass folds the map into a constant, as before.
  • Data-size limits: the walker and the Grain VM start a map literal's running total from its template's sizes, so the constant values still count. The template has a () for each computed value, so every entry is counted once, as before.
  • Parser: the duplicate-property check looks the key up in the template instead of scanning the entry list. This roughly halves parse time for a 1000-key literal.
  • Debug for Expr::Map shows the moved constants.

Why the optimizer and not the parser

Moving the constants in the parser breaks test_max_operations_literal, which requires each constant entry of a map literal to count as an operation under OptimizationLevel::None. Doing it in the optimizer matches how it already folds a map literal that is all constant. Under OptimizationLevel::None, and in no_optimize builds, nothing changes.

Behavior changes (when optimizing)

These are the same as for an all-constant map literal today:

  • Constant entries no longer count as operations, and the debugger does not step into them.
  • If the constant values alone exceed a size limit, the error points at the first computed entry instead of the constant that tipped it over.
  • Under internals, the entries of an optimized Expr::Map hold only the computed values.

Benchmarks

Release build, pinned to one core, median of 11 (synthetic) or 21 (luish) runs; each build run twice.

Evaluating a map literal (per evaluation)

let s = 0; for i in 0..N { let m = #{ ... }; s += m.len(); } s, where the computed values read i and the rest are "none".

map literal walker: main → this PR VM: main → this PR
100 keys, 1 computed 5.7 → 2.0 µs 7.7 → 2.0 µs
1000 keys, 1 computed 116 → 19.5 µs 124 → 19.6 µs
1000 keys, 100 computed 131 → 28 µs 133 → 28 µs
1000 keys, all computed ~120 µs, unchanged ~130 µs, unchanged

Real scripts

The luish completion scripts from #1196 (luish-extra completion/), with a stub sh module:

call walker VM Grain compile
ffmpeg_opts values() 77 → 57 µs 83 → 57 µs 57 → 17.8 ms
slurm_opts spec() 28 → 21.6 µs 30 → 22 µs 29 → 11.2 ms
hts samtools() 2.0 → 1.7 µs 2.5 → 2.1 µs 36 → 21.8 ms

These gain less than the synthetic case because their computed values are calls such as k("..."), which this does not touch.

On its own, Grain compile time is still dominated by the quadratic constant pool that #1202 fixes. With both PRs together, ffmpeg_opts compiles in 0.17 ms (57 ms on main), and a 1000-key literal with one computed value in 0.04 ms (86 ms on main).

The two PRs conflict only in the comment above Expr::Map in the Grain compiler.

Tests

New tests, each confirmed to fail without the part of the change it covers:

  • tests/maps.rs: promoting a property of a map literal whose value was moved into the template.
  • tests/modules.rs: a closure in a map literal keeps the imports of its script.
  • tests/data_size.rs: string and map size limits count the constant values of a map literal.
  • tests/grain/limits.rs: the VM refuses the same literals as the walker, with the same error and position, and agrees on the value under the limits.

Checks

  • cargo test --tests passes with default features, internals,grain, bin-features, no_optimize, unchecked,grain,internals, no_closure,grain,internals, sync,grain,internals, and the two CI feature sets that failed on Grain: index the constant pool by hash #1202.
  • cargo test --lib --features internals,grain passes.
  • cargo fmt --all -- --check passes. Clippy reports nothing on the changed lines.
  • cargo check --lib is warning-free across a feature matrix including no_optimize, no_object, no_index, unchecked and grain combinations. The no_ast,grain build has the same two warnings as main.

🤖 Generated with Claude Code

A map literal with any computed value is not folded into a constant, and
every entry, constant or not, was evaluated each time the literal was.

After optimizing a map literal's entries, the optimizer now moves the
constant values into the literal's template, so only the computed ones
are evaluated. A function pointer stays an entry, as it takes the
environment it is evaluated in, and so does a shared value, which is
flattened as it is evaluated. The promotion of `#{..}.a` and `#{..}["a"]`
looks in the template for a moved value.

The walker and the VM start a map literal's running data size from its
template, so the constant values still count towards the limits.

The parser checks for a duplicated property in the template rather than
scanning the entries.

At OptimizationLevel::None nothing moves, so constant entries still
count as operations there.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@schungx

schungx commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Putting it inside the optimizer instead of the parser would make the parser create large numbers of constant AST nodes for the first version of the AST, only to throw those away after folding into th template. For thousands of properties this would be a drag.

Would it be better to just do it in the parser itself? The optimizer then simply becomes taking the template as the value if there are no non-constant props left.

Subtle differences wrt operations counting etc. can be tolerated, I think, for such an optimization. The test results can be updated to reflect the new behavior.

@schungx

schungx commented Oct 29, 2026

Copy link
Copy Markdown
Collaborator

I have created #1207 which implements it in the parser (in which case you don't create and immediately throw away large numbers of constant properties).

Some subtleties wrt data size counting flushed out existing issues (aka bugs...) in Rhai. These were actually preventing AI from doing a more thorough job to keep with the incorrect behavior... which is why I mentioned that there are subtle complications.

Take that PR as a reference. @luispedro I would be interested to see how my PR benchmarks vs yours (doing it as an optimizer step).

@schungx

schungx commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator

I'm going to merge #1127 as a preferred implementation. Closing this now.

@schungx schungx closed this Oct 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants