Skip to content

perf(json): use FxHashSet for cycle detection in JSON.stringify#5145

Open
iammdzaidalam wants to merge 4 commits intoboa-dev:mainfrom
iammdzaidalam:perf/json-stringify-cycle-set
Open

perf(json): use FxHashSet for cycle detection in JSON.stringify#5145
iammdzaidalam wants to merge 4 commits intoboa-dev:mainfrom
iammdzaidalam:perf/json-stringify-cycle-set

Conversation

@iammdzaidalam
Copy link
Copy Markdown
Contributor

Description

JSON.stringify currently performs cycle detection using linear scans over the active stack.

This replaces those checks with an FxHashSet, while keeping the existing traversal order in a Vec.

StateRecord now stores:

  • stack: Vec<JsObject>
  • stack_set: FxHashSet<JsObject>

Both are updated together in serialize_json_object and serialize_json_array.

Behavior remains unchanged, only the lookup strategy is different.

Changes

  • use FxHashSet for cycle detection
  • keep Vec for order
  • ensure both structures stay in sync during push/pop

Tests

Added tests for:

  • cyclic object
  • cyclic array
  • nested cyclic structures

Notes

Ran local checks (fmt, check, clippy, tests).

Tried running cargo bench -p boa_benches, but the suite hit a stack overflow in v8-benches/deltablue, so not using it as validation for this change.

@iammdzaidalam iammdzaidalam requested a review from a team as a code owner March 19, 2026 01:46
@github-actions github-actions bot added Waiting On Review Waiting on reviews from the maintainers C-Tests Issues and PRs related to the tests. C-Builtins PRs and Issues related to builtins/intrinsics and removed Waiting On Review Waiting on reviews from the maintainers labels Mar 19, 2026
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 19, 2026
@github-actions
Copy link
Copy Markdown

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 50,073 50,073 0
Ignored 2,072 2,072 0
Failed 818 818 0
Panics 0 0 0
Conformance 94.54% 94.54% 0.00%

Tested main commit: 055ee0958ce332f3f99af27536a7c6f9a91def27
Tested PR commit: b41109eafe7beaa5dae30222bb2957e5ec3021eb
Compare commits: 055ee09...b41109e

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 19, 2026

Codecov Report

❌ Patch coverage is 71.42857% with 12 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.80%. Comparing base (6ddc2b4) to head (3fbccc7).
⚠️ Report is 914 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/builtins/json/mod.rs 71.42% 12 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5145       +/-   ##
===========================================
+ Coverage   47.24%   59.80%   +12.56%     
===========================================
  Files         476      582      +106     
  Lines       46892    63490    +16598     
===========================================
+ Hits        22154    37972    +15818     
- Misses      24738    25518      +780     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@iammdzaidalam
Copy link
Copy Markdown
Contributor Author

Hi @jedel1043, small perf change for JSON cycle detection, would appreciate a review.

@zhuzhu81998
Copy link
Copy Markdown
Contributor

zhuzhu81998 commented Mar 19, 2026

Tried running cargo bench -p boa_benches, but the suite hit a stack overflow in v8-benches/deltablue, so not using it as validation for this change.

a simple test that actually uses JSON.stringify and measures the time would be better than nothing.
not sure if the current benches use json functions at all.

@iammdzaidalam
Copy link
Copy Markdown
Contributor Author

Yeah, that makes sense.

If boa_benches is not exercising JSON.stringify directly, it is not a very useful signal for this change. I'll add a small focused benchmark for it.

@github-actions github-actions bot added Waiting On Review Waiting on reviews from the maintainers C-Benchmark Issues and PRs related to the benchmark subsystem. C-Javascript Pull requests that update Javascript code and removed Waiting On Review Waiting on reviews from the maintainers labels Mar 19, 2026
@iammdzaidalam
Copy link
Copy Markdown
Contributor Author

Added focused JSON.stringify benchmarks for this path:

  • stringify_deep.js (deep acyclic case)
  • stringify_circular.js (cycle detection path)

I tried running them with cargo bench -p boa_benches -- json/..., but the current bench harness hits a pre-existing stack overflow before the filter takes effect. It looks like all script benches are initialized first, so unrelated v8-benches entries fail before reaching these.

Left that as a separate follow-up rather than expanding this PR.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 19, 2026

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 50,545 50,545 0
Ignored 1,426 1,426 0
Failed 992 992 0
Panics 2 2 0
Conformance 95.43% 95.43% 0.00%

Tested main commit: 817b6eaade662a49434c5d295667068a4875b378
Tested PR commit: 3fbccc7a630920fb70bb49988d155a8121d22403
Compare commits: 817b6ea...3fbccc7

@zhuzhu81998
Copy link
Copy Markdown
Contributor

Added focused JSON.stringify benchmarks for this path:

* `stringify_deep.js` (deep acyclic case)

* `stringify_circular.js` (cycle detection path)

I tried running them with cargo bench -p boa_benches -- json/..., but the current bench harness hits a pre-existing stack overflow before the filter takes effect. It looks like all script benches are initialized first, so unrelated v8-benches entries fail before reaching these.

Left that as a separate follow-up rather than expanding this PR.

then dont use the cargo bench. run the script manually and time it (remember to add a loop though). and compare the time between pr and main.

@iammdzaidalam
Copy link
Copy Markdown
Contributor Author

yeah, fair. will just time it manually with a loop on main vs this PR and post numbers.

@iammdzaidalam
Copy link
Copy Markdown
Contributor Author

Ran the scripts manually through boa_cli instead of cargo bench, using the same files on both main and this branch and timing the built binary directly.

On my machine (after warmup), timings were roughly:

Deep case:

  • PR: ~14.3–17.4 ms
  • main: ~15.5–18.3 ms

Circular case:

  • PR: ~14.5–16.8 ms
  • main: ~14.4–17.1 ms

The deep acyclic case shows a small improvement on this branch, while the circular case is roughly flat and within noise. This is just a quick manual check, not a rigorous benchmark.

@iammdzaidalam iammdzaidalam force-pushed the perf/json-stringify-cycle-set branch from 7d5d594 to 67e5836 Compare March 20, 2026 19:21
@iammdzaidalam
Copy link
Copy Markdown
Contributor Author

Benchmark

Ran the focused stringify scripts manually through boa_cli --release (10 rounds each, after warmup).

Deep case (acyclic object)

Metric main PR Improvement
Typical ~46.6–47.0s ~42.3–42.7s ~9–10% faster
Min ~46.6s ~42.3s ~9.2% faster
Max ~50.3s ~46.4s ~7–8% faster

Circular case (cycle detection)

Metric main PR Improvement
Typical ~1.69–1.72s ~1.38–1.39s ~15–20% faster
Min ~1.69s ~1.36s ~19.5% faster
Max ~1.79s ~1.40s ~21.8% faster

Notes

  • measured using the same scripts on both branches
  • each run uses 10 rounds after warmup
  • numbers are approximate ranges from manual runs (not a rigorous benchmark)

@iammdzaidalam iammdzaidalam force-pushed the perf/json-stringify-cycle-set branch from 67e5836 to 3fbccc7 Compare March 21, 2026 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Benchmark Issues and PRs related to the benchmark subsystem. C-Builtins PRs and Issues related to builtins/intrinsics C-Javascript Pull requests that update Javascript code C-Tests Issues and PRs related to the tests. Waiting On Review Waiting on reviews from the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants