Skip to content

Parenthesize expressions to correctly pre-allocate List<T>#2501

Merged
lahma merged 2 commits into
sebastienros:mainfrom
jnyrup:coalesce
Jun 8, 2026
Merged

Parenthesize expressions to correctly pre-allocate List<T>#2501
lahma merged 2 commits into
sebastienros:mainfrom
jnyrup:coalesce

Conversation

@jnyrup

@jnyrup jnyrup commented Jun 7, 2026

Copy link
Copy Markdown
Contributor

Summary

Parenthesize expressions to correctly pre-allocate List<T>.

Details

As + binds stronger than ?? this PR parenthesizes the coalescing parts.

See this SharpLab Example for how missing parentheses affect the result.

Linked issue

Test plan

  • [N/A] Added or updated unit tests in Jint.Tests
  • Ran dotnet test --configuration Release locally
  • [N/A] For ECMAScript spec changes: ran Jint.Tests.Test262 and confirmed no regressions
  • [N/A] For interop changes: covered in Jint.Tests/Runtime/Interop
  • For perf changes: included before/after numbers from Jint.Benchmark
    • Can you advice which benchmark to run?

Breaking change?

No

@lahma

lahma commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Can you describe the net effect?

@jnyrup

jnyrup commented Jun 7, 2026

Copy link
Copy Markdown
Contributor Author

Can you describe the net effect?

Potentially fewer resizes of List<T>.

E.g. for the first change, temp?.Length ?? 0 + 1 is currently evaluated as temp?.Length ?? (0 + 1) so we either specify a capacity of temp.Length or 1.
When adding temp.Length + 1 elements to properties, it might cause properties having to resize.

With the proposed change to (temp?.Length ?? 0) + 1 , we specify a capacity of temp.Length + 1 or 1

The capacity hint only accounted for the dense backing store
(temp?.Length); in sparse mode temp is null, so the List was sized to 1
and regrew while adding the _sparse entries. Use _sparse!.Count for that
branch, matching the existing '_dense?.Length ?? _sparse!.Count' idiom
used elsewhere in the file.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@lahma

lahma commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

I pushed a follow-up commit (375cb8f) on top of this. While benchmarking I noticed the ArrayInstance.GetOwnPropertyKeys capacity has the same class of bug in its other branch: in sparse mode _dense (temp) is null, so even after your parenthesization the capacity collapses to (null ?? 0) + 1 == 1 and the list regrows while adding every _sparse entry. The commit sizes that branch from _sparse!.Count, matching the existing _dense?.Length ?? _sparse!.Count idiom used elsewhere in the file.

Which benchmark

There isn't an existing one that isolates these paths, so I wrote a focused [MemoryDiagnoser] benchmark hitting the three fixed sites. Since List<T> capacity is purely a sizing hint, the effect is allocation / GC pressure, not wall-clock — so the Allocated column is the signal, and it's deterministic. Wall-clock moved down slightly on all three but stays within the error bars.

before = base main, after = this PR head incl. the follow-up. AMD Ryzen 9 5950X, .NET 10.0.8, ShortRunJob.

Method (per call) Allocated before Allocated after Δ Gen0/1k before → after
Object.keys — dense array (256) 8.95 KB 4.93 KB −45 % 0.542 → 0.298
Object.keys — sparse array (500) 13.03 KB 8.74 KB −33 % 0.794 → 0.534
Reflect.ownKeys — numeric + symbol object 4.24 KB 2.78 KB −34 % 0.244 → 0.153

Mapping to the changes:

  • dense — your (temp?.Length ?? 0) + 1 fix: the backing store length equals the element count for an array literal, so adding the non-enumerable length key tipped the list one past capacity and forced a 256 → 512 realloc. Now pre-sized.
  • sparse — the follow-up commit (_sparse!.Count): removes the regrow chain from capacity 1.
  • numeric + symbol — your ObjectInstance fix: the sorted path's old capacity dropped the symbol and initial-key counts, so adding the symbols forced a regrow. Now the sum is exact.
benchmark source (ad-hoc, not committed)
[MemoryDiagnoser]
[ShortRunJob]
public class GetOwnPropertyKeysBenchmark
{
    private Engine _engine = null!;
    private Prepared<Script> _denseKeys, _sparseKeys, _numericOwnKeys;

    [GlobalSetup]
    public void Setup()
    {
        _engine = new Engine();
        _engine.Evaluate("var denseArr = [" + string.Join(",", Enumerable.Range(0, 256)) + "];");
        _engine.Evaluate("var sparseArr = []; for (var i = 0; i < 500; i++) Object.defineProperty(sparseArr, i, { value: i, enumerable: true, configurable: true, writable: true });");
        _engine.Evaluate("var numObj = {}; for (var i = 0; i < 100; i++) numObj[i] = i; for (var j = 0; j < 16; j++) numObj[Symbol('s' + j)] = j;");
        _denseKeys = Engine.PrepareScript("Object.keys(denseArr);");
        _sparseKeys = Engine.PrepareScript("Object.keys(sparseArr);");
        _numericOwnKeys = Engine.PrepareScript("Reflect.ownKeys(numObj);");
    }

    [Benchmark] public JsValue DenseArrayKeys() => _engine.Evaluate(_denseKeys);
    [Benchmark] public JsValue SparseArrayKeys() => _engine.Evaluate(_sparseKeys);
    [Benchmark] public JsValue NumericObjectOwnKeys() => _engine.Evaluate(_numericOwnKeys);
}

@lahma
lahma enabled auto-merge (squash) June 8, 2026 12:23
@lahma
lahma merged commit 5c702b4 into sebastienros:main Jun 8, 2026
4 checks passed
@jnyrup
jnyrup deleted the coalesce branch June 8, 2026 12:26
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