Skip to content

Inline fits_element in formatter#26429

Merged
charliermarsh merged 1 commit into
mainfrom
charlie/codex-codspeed-5pct
Jun 27, 2026
Merged

Inline fits_element in formatter#26429
charliermarsh merged 1 commit into
mainfrom
charlie/codex-codspeed-5pct

Conversation

@charliermarsh

@charliermarsh charliermarsh commented Jun 27, 2026

Copy link
Copy Markdown
Member

Summary

The formatter's fitting loop calls FitsMeasurer::fits_element for every visited format element. LLVM does not inline this dispatcher heuristically because of its size, so each visit currently pays for an out-of-line function call.

This change marks fits_element as #[inline(always)], keeping the dispatcher in the hot fitting loop. On the non-microbenchmark formatter[large/dataset.py], CodSpeed CPU simulation improves from 8.42 ms to 7.98 ms, a 5.5% speedup (comparison). The other measured formatter workloads improve by 1.0-4.5%, with no formatter output or memory changes.

@astral-sh-bot

astral-sh-bot Bot commented Jun 27, 2026

Copy link
Copy Markdown

ruff-ecosystem results

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

ℹ️ ecosystem check encountered format errors. (no format changes; 1 project error)

ibis-project/ibis (error)

ruff format --preview

warning: Detected debug build without --no-cache.
error: Encountered error: No such file or directory (os error 2)
error: Encountered error: No such file or directory (os error 2)

@codspeed-hq

codspeed-hq Bot commented Jun 27, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 4.94%

⚡ 3 improved benchmarks
✅ 57 untouched benchmarks
⏩ 91 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation formatter[large/dataset.py] 8.4 ms 7.9 ms +5.67%
Simulation formatter[pydantic/types.py] 3.3 ms 3.2 ms +4.65%
Simulation formatter[unicode/pypinyin.py] 638.8 µs 611.1 µs +4.52%

Tip

Curious why this is faster? Use the CodSpeed MCP and ask your agent.


Comparing charlie/codex-codspeed-5pct (87263b3) with main (fa4a7bc)

Open in CodSpeed

Footnotes

  1. 91 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@MichaReiser MichaReiser left a comment

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.

Nice (ignoring the title)

@charliermarsh charliermarsh added performance Potential performance improvement formatter Related to the formatter labels Jun 27, 2026
@charliermarsh charliermarsh changed the title [codex] Improve formatter fitting performance Speed up formatter fitting Jun 27, 2026
@charliermarsh
charliermarsh marked this pull request as ready for review June 27, 2026 16:23
@charliermarsh charliermarsh changed the title Speed up formatter fitting Inline fits_element in formatter Jun 27, 2026
@charliermarsh
charliermarsh merged commit 7af2c7c into main Jun 27, 2026
75 of 77 checks passed
@charliermarsh
charliermarsh deleted the charlie/codex-codspeed-5pct branch June 27, 2026 16:23
graphite-app Bot pushed a commit to oxc-project/oxc that referenced this pull request Jun 30, 2026
## Summary

- inline the formatter-core `FitsMeasurer::fits_element` dispatcher
- mirrors the applicable hot-loop optimization from astral-sh/ruff#26429

## Local benchmark

Command:

```sh
cargo bench -p oxc_benchmark --bench formatter --no-default-features --features compiler -- --sample-size 20 --warm-up-time 1 --measurement-time 3
```

Compared `main` (`d05773604f`) against this branch (`356e65db1c`) with Criterion saved baselines. Median times from a single local run:

| fixture | main median | PR median | delta |
| --- | ---: | ---: | ---: |
| `RadixUIAdoptionSection.jsx` | 41.82 us | 39.69 us | -5.09% |
| `errors.ts` | 60.51 us | 56.20 us | -7.12% |
| `Search.tsx` | 249.52 us | 239.56 us | -3.99% |
| `core.js` | 230.19 us | 221.52 us | -3.77% |
| `next.ts` | 360.07 us | 348.98 us | -3.08% |
| `index.tsx` | 712.25 us | 673.43 us | -5.45% |
| `handle-comments.js` | 394.35 us | 376.84 us | -4.44% |
| `types.ts` | 1.393 ms | 1.304 ms | -6.42% |
| `App.tsx` | 8.928 ms | 8.156 ms | -8.64% |

Geomean across these formatter fixtures: -5.35% median runtime. Treat as directional local data; CI/CodSpeed is the source of truth.
charliermarsh added a commit that referenced this pull request Jul 2, 2026
## Summary

The formatter calls `Printer::print_element` for every rendered format
element and `Printer::print_text` for every text element. LLVM leaves
both private methods out of line even when they are marked `#[inline]`
and the release profile uses fat LTO, so these hot loops retain
out-of-line calls.

This marks both functions as `#[inline(always)]`, allowing the printing
loop and its common text paths to optimize together. It follows the same
general source-level inlining approach as #26429 and closed #26473, but
targets the printing pass rather than fitting or comment extraction. The
methods have a small, fixed set of call sites, and the release binary's
`.text` grows by 11,264 bytes (0.36%).

## Performance

To test whether ordinary `#[inline]` is sufficient and whether LTO
changes the result, I built the exact base with no annotation,
`#[inline]`, and `#[inline(always)]` under both the profiling profile
(LTO disabled) and the release profile (fat LTO). Plain `#[inline]`
neither removed the out-of-line symbols nor changed `.text`, and
CodSpeed reported 0.0% in both profiles. Forced inlining removed both
symbols and preserved the improvement under fat LTO:

| Profile | Annotation | Overall impact | `.text` delta |
| --- | --- | ---: | ---: |
| Profiling (no LTO) | `#[inline]` | 0.0% | 0 bytes |
| Profiling (no LTO) | `#[inline(always)]` | +6.2% | +11,152 bytes
(+0.35%) |
| Release (fat LTO) | `#[inline]` | 0.0% | 0 bytes |
| Release (fat LTO) | `#[inline(always)]` | +6.0% | +11,264 bytes
(+0.36%) |

The release/fat-LTO comparison on the existing non-microbenchmark
formatter suite reports:

| Benchmark | Base | Head | Speedup |
| --- | ---: | ---: | ---: |
| `formatter[large/dataset.py]` | 7.53 ms | 7.02 ms | 7.3% |
| `formatter[pydantic/types.py]` | 2.99 ms | 2.79 ms | 7.2% |
| `formatter[numpy/ctypeslib.py]` | 1.57 ms | 1.48 ms | 5.9% |
| `formatter[unicode/pypinyin.py]` | 580.30 µs | 550.17 µs | 5.5% |
| `formatter[numpy/globals.py]` | 217.95 µs | 209.04 µs | 4.3% |
camc314 added a commit to oxc-project/oxc that referenced this pull request Jul 3, 2026
## Summary

- inline the formatter-core `FitsMeasurer::fits_element` dispatcher
- mirrors the applicable hot-loop optimization from astral-sh/ruff#26429

## Local benchmark

Command:

```sh
cargo bench -p oxc_benchmark --bench formatter --no-default-features --features compiler -- --sample-size 20 --warm-up-time 1 --measurement-time 3
```

Compared `main` (`d05773604f`) against this branch (`356e65db1c`) with Criterion saved baselines. Median times from a single local run:

| fixture | main median | PR median | delta |
| --- | ---: | ---: | ---: |
| `RadixUIAdoptionSection.jsx` | 41.82 us | 39.69 us | -5.09% |
| `errors.ts` | 60.51 us | 56.20 us | -7.12% |
| `Search.tsx` | 249.52 us | 239.56 us | -3.99% |
| `core.js` | 230.19 us | 221.52 us | -3.77% |
| `next.ts` | 360.07 us | 348.98 us | -3.08% |
| `index.tsx` | 712.25 us | 673.43 us | -5.45% |
| `handle-comments.js` | 394.35 us | 376.84 us | -4.44% |
| `types.ts` | 1.393 ms | 1.304 ms | -6.42% |
| `App.tsx` | 8.928 ms | 8.156 ms | -8.64% |

Geomean across these formatter fixtures: -5.35% median runtime. Treat as directional local data; CI/CodSpeed is the source of truth.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

formatter Related to the formatter performance Potential performance improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants