Skip to content

KCL: Avoid recasting array items twice #7876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

adamchalmers
Copy link
Contributor

@adamchalmers adamchalmers commented Jul 22, 2025

Before this PR, array expression recasting was a bit slow. The reason was that there's two ways to recast an array:

  • Single line array string (all items on one line)
  • Multi-line array string (n lines, one per item)

The steps were basically:

  1. Recast each item, collecting into a vec with n strings (one string per item)
  2. Concat the items into a single-line array string
  3. If the line is too long, discard it and do a multi-line array string.

Giving each item its own string meant we were doing N allocations (one per item) but it meant that each item was only recast once.

This PR has a better approach. We build a single-line array first, and if it's too long, then ignore it and do a multi-line array. HOWEVER, the difference is that we track each item's offset within the single-line array string. If we then need to convert it into a multi-line array string, we can do that easily by looking up each item's offset.

Improves benchmarks by 7% and 20% on my macbook pro. 15.6% on Codspeed.

@adamchalmers adamchalmers requested a review from a team as a code owner July 22, 2025 02:51
Copy link

vercel bot commented Jul 22, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
modeling-app ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 22, 2025 3:10am

@adamchalmers adamchalmers force-pushed the achalmers/faster-array-recast branch from c8a37a7 to 0197e01 Compare July 22, 2025 02:55
Before this PR, array expression recasting was a bit slow. The reason was that there's two ways to recast an array:

 - Single line array string (all items on one line)
 - Multi-line array string (n lines, one per item)

The steps were basically:

 1. Recast each item, collecting into a vec with n strings (one string per item)
 2. Concat the items into a single-line array string
 3. If the line is too long, discard it and do a multi-line array string.

Giving each item its own string meant we were doing N allocations (one per item) but it meant that each item was only recast once.

This PR has a better approach. We build a single-line array first, and if it's too long, then ignore it and do a multi-line array. HOWEVER, the difference is that we track each item's offset within the single-line array string. If we then need to convert it into a multi-line array string, we can do that easily by looking up each item's offset.

Improves benchmarks by 7% and 20% on my macbook pro.
Copy link

codspeed-hq bot commented Jul 22, 2025

CodSpeed Instrumentation Performance Report

Merging #7876 will improve performances by 15.64%

Comparing achalmers/faster-array-recast (b9aa82d) with main (d8a4ac7)

Summary

⚡ 3 improvements
✅ 86 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
mock_execute_mike_stress_test_program 316.5 ms 273.6 ms +15.64%
recast_medium_sketch 178.5 µs 160.4 µs +11.29%
recast_mike_stress_test_program 2.6 ms 2.4 ms +11.26%

@jtran
Copy link
Contributor

jtran commented Jul 22, 2025

What you're saying is that the single-line representation of an item must be the same as the multi-line representation. Think about how it interacts with nested arrays, for example. Basically, we should be shooting for pretty printing based on max line width, and this algorithm moves even further from that.

@jtran
Copy link
Contributor

jtran commented Jul 22, 2025

Also, does this handle the case when a nested array item is multiline? It should force the outer array to be multiline, right?

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