Skip to content

Replace the String producing Display logic to using a Formatter#4393

Merged
nekevss merged 1 commit intoboa-dev:mainfrom
hansl:value-display-formatter
Sep 5, 2025
Merged

Replace the String producing Display logic to using a Formatter#4393
nekevss merged 1 commit intoboa-dev:mainfrom
hansl:value-display-formatter

Conversation

@hansl
Copy link
Contributor

@hansl hansl commented Aug 28, 2025

This code heavily relied on allocating strings and vectors of strings for each object properties. This new logic eliminates the allocations entirely, saving both memory and improving performance.

It also makes the logic easier to follow, which might be help with simplification later.

Tests should be unchanged.

This code heavily relied on allocating strings and vectors of strings
for each object properties. This new logic eliminates the allocations
entirely, saving both memory and improving performance.

It also makes the logic easier to follow, which might be help with
simplification later.

Tests should be unchanged.
@codecov
Copy link

codecov bot commented Aug 28, 2025

Codecov Report

❌ Patch coverage is 63.15789% with 42 lines in your changes missing coverage. Please review.
✅ Project coverage is 50.27%. Comparing base (6ddc2b4) to head (a7389c7).
⚠️ Report is 517 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/value/display.rs 63.15% 42 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4393      +/-   ##
==========================================
+ Coverage   47.24%   50.27%   +3.03%     
==========================================
  Files         476      508      +32     
  Lines       46892    51144    +4252     
==========================================
+ Hits        22154    25714    +3560     
- Misses      24738    25430     +692     

☔ 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.

@hansl
Copy link
Contributor Author

hansl commented Aug 28, 2025

Not sure how much console logging is done in the benchmark tests, but I did notice a noticeable (reproduced twice, but within noise) increase in performance:

Before PR

PROGRESS Richards
RESULT Richards 174
PROGRESS DeltaBlue
RESULT DeltaBlue 194
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 161
PROGRESS RayTrace
RESULT RayTrace 439
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 530
PROGRESS RegExp
RESULT RegExp 85.7
PROGRESS Splay
RESULT Splay 809
PROGRESS NavierStokes
RESULT NavierStokes 358
SCORE 274

With PR

PROGRESS Richards
RESULT Richards 179
PROGRESS DeltaBlue
RESULT DeltaBlue 196
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 167
PROGRESS RayTrace
RESULT RayTrace 445
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 538
PROGRESS RegExp
RESULT RegExp 85.0
PROGRESS Splay
RESULT Splay 796
PROGRESS NavierStokes
RESULT NavierStokes 365
SCORE 277

@hansl hansl requested a review from a team August 28, 2025 17:49
@nekevss
Copy link
Member

nekevss commented Aug 28, 2025

Have you seen the writeable crate by chance?

@hansl
Copy link
Contributor Author

hansl commented Sep 4, 2025

@nekevss I had not, let me see if it helps push performance further.

@hansl
Copy link
Contributor Author

hansl commented Sep 4, 2025

@nekevss I didn't see performance improvements (even saw minor regression). I cannot inspect values and guess the size of output as I need a context, so I don't see any theoretical gains from using writeable.

@nekevss nekevss added this pull request to the merge queue Sep 5, 2025
Merged via the queue into boa-dev:main with commit 9847e29 Sep 5, 2025
16 checks passed
@hansl hansl deleted the value-display-formatter branch September 5, 2025 17:15
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