Skip to content

feat(rhai): add intern_strings config option to allow elimination of RwLock contention under concurrent load (copy #9070)#9146

Open
mergify[bot] wants to merge 12 commits intodevfrom
mergify/copy/dev/pr-9070
Open

feat(rhai): add intern_strings config option to allow elimination of RwLock contention under concurrent load (copy #9070)#9146
mergify[bot] wants to merge 12 commits intodevfrom
mergify/copy/dev/pr-9070

Conversation

@mergify
Copy link
Copy Markdown
Contributor

@mergify mergify bot commented Apr 7, 2026

Closes #9070

The router's Rhai engine is compiled with the sync feature (required for multi-threaded operation), which causes string interning to be protected by a RwLock<StringsInterner>. Under high concurrency, threads that encounter new strings must acquire a write lock, which serializes those operations across all concurrent requests and can become a throughput bottleneck in script-heavy workloads.

This PR adds an intern_strings: bool field to the Rhai plugin configuration. Setting it to false calls engine.set_max_strings_interned(0), which sets the interner field to None and eliminates the RwLock path entirely. The default (true) preserves Rhai's existing behaviour of 256 interned strings for full backward compatibility — no existing configuration is affected.

Configuration

rhai:
  scripts: ./rhai
  main: main.rhai
  # Set to false to disable string interning and eliminate RwLock contention
  # under concurrent load. See documentation for trade-offs.
  intern_strings: false

Benchmark results

Measured directly against the Rhai engine (no router stack overhead) using a script representative of typical subgraph request callback work: context map lookups with long string keys, in-operator containment checks, template-string interpolation, string comparisons, and a cookie-building loop.

Variant Default (256 interned) Disabled (0 interned) Improvement
Sequential (1 thread) 14.02 µs 14.00 µs ~0% (within noise)
Concurrent (8 threads) 18.90 µs · 52.9K req/s 7.77 µs · 128.8K req/s ~59% faster · 2.4× throughput

The sequential result shows the RwLock is nearly free when uncontested. The concurrent result shows it becomes a genuine serialization point under load — disabling interning removes that bottleneck entirely.

The benchmark is included in apollo-router-benchmarks/benches/rhai_string_interning.rs and can be run with:

cargo bench -p apollo-router-benchmarks --bench rhai_string_interning

Trade-offs

Disabling interning increases per-string allocation pressure (repeated identical strings are no longer shared) and removes the pointer-equality fast path for string comparisons. For low-concurrency deployments these costs may outweigh the benefit. The documentation covers this in detail.


Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

  • PR description explains the motivation for the change and relevant context for reviewing
  • PR description links appropriate GitHub/Jira tickets (creating when necessary)
  • Changeset is included for user-facing changes
  • Changes are compatible1
  • Documentation2 completed
  • Performance impact assessed and acceptable
  • Metrics and logs are added3 and documented
  • Tests added and passing4
    • Unit tests
    • Integration tests
    • Manual tests, as necessary

Exceptions

No new metrics or logs — this is a configuration option that controls an existing engine behaviour with no observable side-effects beyond performance characteristics.

No integration tests — the feature is covered by unit tests exercising the full config deserialization → plugin initialization path (it_creates_plugin_with_intern_strings_false) and a deny_unknown_fields rejection test, in addition to the engine-level behavioural tests.

Notes


This is an automatic copy of pull request #9070 done by [Mergify](https://mergify.com).

Footnotes

  1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.

  2. Configuration is an important part of many changes. Where applicable please try to document configuration examples.

  3. A lot of (if not most) features benefit from built-in observability and debug-level logs. Please read this guidance on metrics best-practices.

  4. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions.

theJC and others added 5 commits April 7, 2026 09:59
…interning

When the router's Rhai engine is compiled with the `sync` feature (required
for multi-threaded use), string interning is protected by a
`RwLock<StringsInterner>`. Under high concurrency every string operation that
encounters a new string must acquire a write lock, serializing those
operations across all concurrent requests.

Adds a `max_strings_interned: Option<usize>` field to the Rhai plugin
configuration. Setting it to `0` calls `engine.set_max_strings_interned(0)`,
which sets the interner to `None` and eliminates the lock path entirely.
The default (`null`) preserves Rhai's existing behaviour of 256 interned
strings for full backward compatibility.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit 12ca04d)
- Remove articles before standalone product names (the Rhai/Router → Rhai/Router)
- Remove quotes from section title reference in YAML comment
- 'may' → 'might' for potential occurrences (2 instances)
- 'a large number of' → 'many'
- 'amortizing allocation costs' → 'spreading out allocation costs'
- 'Cons' → 'Considerations' (neutral framing)
- Remove ending punctuation from list item fragments
- 'before changing' → 'before you change' (active, reader-framed voice)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit f4cd9d8)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit c8ae40e)
…ngs config

Users would only ever set max_strings_interned to 0 (disable) or leave it
at the default (256), making the numeric option unnecessarily complex. A
boolean intern_strings (default: true) is clearer and prevents footguns
from arbitrary values.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit d19f8f4)
… warning in benchmark

- Replace `max_strings_interned: 0` comment in rhai/index.mdx with correct `intern_strings: false`
- Bind benchmark eval result with `let _ =` to suppress unused_must_use compiler warning

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
(cherry picked from commit c24b800)
@mergify mergify bot requested review from a team as code owners April 7, 2026 09:59
@apollo-cla
Copy link
Copy Markdown

@mergify[bot]: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@apollo-librarian
Copy link
Copy Markdown
Contributor

apollo-librarian bot commented Apr 7, 2026

✅ Docs preview ready

The preview is ready to be viewed. View the preview

File Changes

0 new, 1 changed, 0 removed
* graphos/routing/(latest)/customization/rhai/index.mdx

Build ID: bb1e92ca3619d59c877cc24c
Build Logs: View logs

URL: https://www.apollographql.com/docs/deploy-preview/bb1e92ca3619d59c877cc24c


⚠️ AI Style Review — 7 Issues Found

Summary

The documentation has been updated to align with the style guide across four key sections. In 'Framing Apollo Products,' the language was simplified (e.g., using 'use' instead of 'utilize') and reframed to emphasize positive feature advantages rather than the disadvantages of alternatives. Under 'Products and Features,' references to the 'Apollo Router' were corrected to remove unnecessary articles. 'Text Formatting' was improved by replacing vague link text like 'details' with descriptive noun phrases. Finally, the 'Voice' section was updated to adopt a more authoritative and prescriptive tone, specifically when providing guidance for high-concurrency scenarios.

Duration: 2871ms
Review Log: View detailed log

This review is AI-generated. Please use common sense when accepting these suggestions, as they may not always be accurate or appropriate for your specific context.

@theJC
Copy link
Copy Markdown
Contributor

theJC commented Apr 7, 2026

Please make sure @jhrldev is in the loop on progressing merging this change in (ref: TSH-22414)

@theJC
Copy link
Copy Markdown
Contributor

theJC commented Apr 7, 2026

Can we make #9072 a prerequisite for this, so that customers are able to have metrics that could help them make an informed decision on the impact of using this change or not?

@goto-bus-stop goto-bus-stop requested a review from a team April 10, 2026 13:47

This is particularly risky when using closures within callbacks while referencing external data. Take particular care to avoid this kind of situation by making copies of data when required. The [examples/surrogate-cache-key directory](https://github.com/apollographql/router/tree/main/examples/surrogate-cache-key) contains an example of this, where "closing over" `response.headers` would cause a deadlock. To avoid this, a local copy of the required data is obtained and used in the closure.

### String interning and concurrent performance
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apollographql/docs Could you give this section a look? It's quite an in-depth feature and I'm not totally confident on the level of technical detail to give here.

Applied suggestions from AI review 20260410-911566dd-9146-961e53dd87883704:
- docs/source/routing/customization/rhai/index.mdx:555: Use an authoritative and encouraging tone to guide the user toward best practice...
- docs/source/routing/customization/rhai/index.mdx:537: Remove the hyphen from 'frequently-used' as it is not a compound modifier requir...

Review: #9146
Triggered by: renee.kooi@apollographql.com
Copy link
Copy Markdown
Contributor

@carodewig carodewig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks great! Mostly just wondering about whether this should be an opt-out change rather than an opt-in, given the performance results

}

fn default_intern_strings() -> bool {
true
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the performance tests above, is there enough data to suggest that the default should actually be false?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was thinking of landing it like this first, and switch the default at some point in the future. It might be overly cautious?

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.

4 participants