Skip to content

Conversation

@jackoconnordev
Copy link
Contributor

@jackoconnordev jackoconnordev commented Aug 9, 2025

Description

  • Adds a new microbenchmark for sorted (and by extension sort) builtin functions
  • Documents how to run individual benchmarks to the relevant README.

I chose 5 * ITERATIONS to better show the divergence between the sort implementations, while trying to avoid individual microbenchmark runs timing out.

Sample benchmark results

Violin plot
image

Line Chart
image

Manually benchmark

Using larger list sizes really shows the difference. Sorting 1_000_000 random numbers:

  • CPython => 0.3 seconds
  • RustPython => ~17 minutes
✦ ❯ time python -c "from random import random; sorted([random() for i in range(1_000_000)]); print('DONE');"
DONE

real    0m0.309s
user    0m0.274s
sys    0m0.036s

✦ ❯ time cargo run --release -- -c "from random import random; sorted([random() for i in range(1_000_000)]); print('DONE');"
    Finished `release` profile [optimized] target(s) in 0.16s
     Running `target/release/rustpython -c 'from random import random; sorted([random() for i in range(1_000_000)]); print('\''DONE'\'');'`
DONE

real    16m52.217s
user    16m51.926s
sys    0m0.174s

Summary by CodeRabbit

  • Tests

    • Added a deterministic Python microbenchmark for sorting to the benchmark suite.
  • Documentation

    • Documented running benchmark subsets by name (e.g., run specific benchmarks).
    • Noted that a graphical benchmark report is generated after runs.
    • Updated Python invocation example to use python3.13.
    • Minor wording, header, and formatting refinements in the benchmark README.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Aug 9, 2025

Walkthrough

Adds a new deterministic Python microbenchmark for sorting and updates the benches README to document name-based subset benchmarking (cargo bench name matching), a graphical report location, and a Python executable example. No public APIs or benchmarking logic were changed.

Changes

Cohort / File(s) Summary
Docs: Benchmarks README
benches/README.md
Added guidance for running subset benchmarks via name matching (e.g., cargo bench sort), noted graphical report at target/criterion/report/index.html, updated example to PYTHON_SYS_EXECUTABLE=python3.13, and made minor wording/formatting changes.
Benchmark: Python microbenchmark
benches/microbenchmarks/sorted.py
Added deterministic sort microbenchmark: seeds RNG to 0, constructs unsorted_list of 5 * ITERATIONS random floats, and calls sorted(unsorted_list) for timing. Assumes ITERATIONS is defined externally; no result capture or additional error handling.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitch my whiskers at the bench’s pace,
Sorting floats in a tidy race,
Seed set to zero, lists shiver and run,
Name-matched benches, reporting fun.
Thump-thump—I hop, the measurements done. 🐇✨

Tip

🔌 Remote MCP (Model Context Protocol) integration is now available!

Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats.

✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
benches/README.md (1)

10-16: Extend example to include list.sort() microbench and show exact-match usage

Once you add the in-place list.sort() microbenchmark, surface it here. Also consider an exact-match example to avoid unintentionally matching other benchmarks.

Apply:

 ```shell
-cargo bench sort
+cargo bench sort
+# Or, for the in-place list.sort() microbenchmark:
+cargo bench list_sort
+# For an exact match on just "sort":
+cargo bench '^sort$'

</blockquote></details>

</blockquote></details>

<details>
<summary>📜 Review details</summary>

**Configuration used: .coderabbit.yml**
**Review profile: CHILL**
**Plan: Pro**


<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between a9a9e3bf118f6c631ef99fe7174215254f846097 and d96712bd950005d8d813bd3a4d8f44fb0426ec58.

</details>

<details>
<summary>📒 Files selected for processing (2)</summary>

* `benches/README.md` (1 hunks)
* `benches/microbenchmarks/sort.py` (1 hunks)

</details>

<details>
<summary>🧰 Additional context used</summary>

<details>
<summary>📓 Path-based instructions (1)</summary>

<details>
<summary>**/*.py</summary>


**📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)**

> `**/*.py`: Follow PEP 8 style for custom Python code
> Use ruff for linting Python code

Files:
- `benches/microbenchmarks/sort.py`

</details>

</details><details>
<summary>🧠 Learnings (1)</summary>

<details>
<summary>📚 Learning: 2025-07-19T03:16:56.511Z</summary>

Learnt from: CR
PR: RustPython/RustPython#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-07-19T03:16:56.511Z
Learning: Generate documentation with cargo doc --no-deps --all


**Applied to files:**
- `benches/README.md`

</details>

</details><details>
<summary>🧬 Code Graph Analysis (1)</summary>

<details>
<summary>benches/microbenchmarks/sort.py (2)</summary><blockquote>

<details>
<summary>stdlib/src/random.rs (1)</summary>

* `random` (42-45)

</details>
<details>
<summary>vm/src/stdlib/builtins.rs (1)</summary>

* `sorted` (815-820)

</details>

</blockquote></details>

</details><details>
<summary>🪛 Flake8 (7.2.0)</summary>

<details>
<summary>benches/microbenchmarks/sort.py</summary>

[error] 3-3: undefined name 'ITERATIONS'

(F821)

</details>

</details>

</details>

<details>
<summary>⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)</summary>

* GitHub Check: Run snippets and cpython tests (macos-latest)
* GitHub Check: Run snippets and cpython tests on wasm-wasi
* GitHub Check: Run snippets and cpython tests (windows-latest)
* GitHub Check: Run snippets and cpython tests (ubuntu-latest)
* GitHub Check: Run tests under miri
* GitHub Check: Check the WASM package and demo
* GitHub Check: Check Rust code with rustfmt and clippy
* GitHub Check: Run rust tests (ubuntu-latest)
* GitHub Check: Run rust tests (windows-latest)
* GitHub Check: Run rust tests (macos-latest)
* GitHub Check: Ensure compilation on various targets

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
benches/microbenchmarks/sort.py (1)

1-1: Ensure Consistent RNG Seeding Across Microbenchmarks

No other benchmarks in benches/microbenchmarks/ currently seed the RNG. If you’d like deterministic inputs for easier cross-run comparisons, consider adding a fixed seed at the top of benches/microbenchmarks/sort.py:

-from random import random
+from random import random, seed
+seed(0)

This change will align sort.py with a reproducible input setup—apply only if it matches the rest of your benchmarking strategy.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d96712b and 5531ae2.

📒 Files selected for processing (2)
  • benches/README.md (1 hunks)
  • benches/microbenchmarks/sort.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • benches/README.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)

**/*.py: Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

  • benches/microbenchmarks/sort.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.479Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.
📚 Learning: 2025-08-09T22:55:10.479Z
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.479Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.

Applied to files:

  • benches/microbenchmarks/sort.py
📚 Learning: 2025-08-09T22:56:24.504Z
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:1-3
Timestamp: 2025-08-09T22:56:24.504Z
Learning: In RustPython's microbenchmarks (benches/microbenchmarks/*.py), the variable `ITERATIONS` is intentionally used without being defined in the Python files. It is injected by the cargo bench harness at runtime. This pattern should be maintained for consistency across all microbenchmarks, and F821 lint warnings for undefined `ITERATIONS` are expected and acceptable in this context.

Applied to files:

  • benches/microbenchmarks/sort.py
🧬 Code Graph Analysis (1)
benches/microbenchmarks/sort.py (2)
stdlib/src/random.rs (1)
  • random (42-45)
vm/src/builtins/list.rs (1)
  • sort (323-337)
🪛 Flake8 (7.2.0)
benches/microbenchmarks/sort.py

[error] 3-3: undefined name 'ITERATIONS'

(F821)

🔇 Additional comments (2)
benches/microbenchmarks/sort.py (2)

3-3: ITERATIONS is intentionally harness-injected; keep as-is

Acknowledging the established pattern in RustPython microbenchmarks: ITERATIONS is provided by the bench harness. F821 from local linters is expected here and acceptable. No change requested.


7-7: No change required: setup is re-executed per iteration
The harness uses Criterion’s iter_batched_ref/iter_batched APIs with the bench_setup closure, which runs the setup block (including unsorted_list = […]) before each sample. That guarantees each sort operates on a fresh, random list.

• In benches/microbenchmarks.rs, see lines 78–86 (b.iter_batched_ref(|| bench_setup(Some(*idx)), …)) and lines 147–155 (b.iter_batched(|| bench_setup(Some(*idx)), …))
• Each invocation of the setup closure re-executes the code before timing, so no iteration ever sorts an already-sorted list.

@jackoconnordev
Copy link
Contributor Author

Setting the seed is pretty reasonable actually. Will do this next time I'm at my computer

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
benches/microbenchmarks/sort.py (2)

4-4: ITERATIONS is intentionally harness-injected; F821 can be ignored here

Per the microbench pattern in this repo, ITERATIONS is provided by the harness. Keeping it undefined in the file is consistent and expected.


8-8: In-place sort mutates the input; subsequent timed invocations will sort an already-sorted list

Because setup runs once and only the section after “# ---” is timed repeatedly, mutating unsorted_list will cause later iterations to measure best-case sorting of an already-sorted list, skewing results. Prefer a non-mutating call or copy within the timed section.

Apply one of the following:

Option A (preferred, aligns with “benchmark only sorted()” learning):

-unsorted_list.sort()
+sorted(unsorted_list)

Option B (if you explicitly want to time list.sort()):

-unsorted_list.sort()
+unsorted_list.copy().sort()
🧹 Nitpick comments (1)
benches/README.md (1)

17-17: Nit: small wording/style improvement and consistency for shell blocks

  • Prefer “benchmark with” over “bench against” for clarity.
  • Keep shell blocks consistent: either include a $ prompt in both code blocks or neither (the earlier “cargo bench sort” block has no $ ).
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5531ae2 and ccc340d.

📒 Files selected for processing (2)
  • benches/README.md (1 hunks)
  • benches/microbenchmarks/sort.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)

**/*.py: Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

  • benches/microbenchmarks/sort.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.512Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.
📚 Learning: 2025-08-09T22:55:10.512Z
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.512Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.

Applied to files:

  • benches/README.md
  • benches/microbenchmarks/sort.py
📚 Learning: 2025-08-09T22:56:24.527Z
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:1-3
Timestamp: 2025-08-09T22:56:24.527Z
Learning: In RustPython's microbenchmarks (benches/microbenchmarks/*.py), the variable `ITERATIONS` is intentionally used without being defined in the Python files. It is injected by the cargo bench harness at runtime. This pattern should be maintained for consistency across all microbenchmarks, and F821 lint warnings for undefined `ITERATIONS` are expected and acceptable in this context.

Applied to files:

  • benches/microbenchmarks/sort.py
🪛 LanguageTool
benches/README.md

[grammar] ~10-~10: There might be a mistake here.
Context: ...ng to run a subset of the benchmarks. To run only the sort microbenchmark, you ca...

(QB_NEW_EN)


[grammar] ~17-~17: There might be a mistake here.
Context: ...rgo bench`. You can also bench against a specific installed Python version by run...

(QB_NEW_EN)

🪛 Flake8 (7.2.0)
benches/microbenchmarks/sort.py

[error] 4-4: undefined name 'ITERATIONS'

(F821)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Ensure compilation on various targets
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Check the WASM package and demo
🔇 Additional comments (2)
benches/microbenchmarks/sort.py (1)

1-2: Good call seeding RNG for reproducibility

Seeding ensures deterministic inputs across runs, which is important for comparing interpreters/engines.

benches/README.md (1)

10-16: Subset benchmark instructions look good

Clear, minimal, and consistent with Criterion’s name filtering. Nice addition.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
benches/microbenchmarks/sort.py (1)

4-4: F821 (undefined name ‘ITERATIONS’) is intentional per harness injection

Per prior discussion and existing microbench patterns, ITERATIONS is injected by the harness; no inline default or noqa is required here.

🧹 Nitpick comments (3)
benches/README.md (2)

13-15: Fix markdownlint MD014: remove leading $ in shell blocks

Use plain commands in fenced blocks to satisfy MD014 and improve copy/paste UX.

 ```shell
-$ cargo bench sort
+cargo bench sort

---

`17-22`: **Minor grammar + markdownlint MD014 cleanups**

- Prefer “benchmark against” over “bench against”.
- Remove leading “$” in shell example for MD014.


```diff
-To view Python tracebacks during benchmarks, run `RUST_BACKTRACE=1 cargo bench`. You can also bench against a
+To view Python tracebacks during benchmarks, run `RUST_BACKTRACE=1 cargo bench`. You can also benchmark against a
 ```shell
-$ PYTHON_SYS_EXECUTABLE=python3.13 cargo bench
+PYTHON_SYS_EXECUTABLE=python3.13 cargo bench

</blockquote></details>
<details>
<summary>benches/microbenchmarks/sort.py (1)</summary><blockquote>

`1-4`: **Avoid mutating global RNG state: use a local Random(0) instance**

Seeding the global RNG can affect other benchmarks if they share a process. A local RNG preserves reproducibility without global side effects.


```diff
-from random import random, seed
-seed(0)
+from random import Random
+rng = Random(0)
 
-unsorted_list = [random() for _ in range(5 * ITERATIONS)]
+unsorted_list = [rng.random() for _ in range(5 * ITERATIONS)]
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ccc340d and fb3dd8f.

📒 Files selected for processing (2)
  • benches/README.md (1 hunks)
  • benches/microbenchmarks/sort.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)

**/*.py: Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

  • benches/microbenchmarks/sort.py
🧠 Learnings (3)
📓 Common learnings
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.512Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.
📚 Learning: 2025-08-09T22:55:10.512Z
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.512Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.

Applied to files:

  • benches/README.md
  • benches/microbenchmarks/sort.py
📚 Learning: 2025-08-09T22:56:24.527Z
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:1-3
Timestamp: 2025-08-09T22:56:24.527Z
Learning: In RustPython's microbenchmarks (benches/microbenchmarks/*.py), the variable `ITERATIONS` is intentionally used without being defined in the Python files. It is injected by the cargo bench harness at runtime. This pattern should be maintained for consistency across all microbenchmarks, and F821 lint warnings for undefined `ITERATIONS` are expected and acceptable in this context.

Applied to files:

  • benches/microbenchmarks/sort.py
🪛 LanguageTool
benches/README.md

[grammar] ~10-~10: There might be a mistake here.
Context: ...ng to run a subset of the benchmarks. To run only the sort microbenchmark, you ca...

(QB_NEW_EN)


[grammar] ~17-~17: There might be a mistake here.
Context: ...rgo bench`. You can also bench against a specific installed Python version by run...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.17.2)
benches/README.md

14-14: Dollar signs used before commands without showing output

(MD014, commands-show-output)


21-21: Dollar signs used before commands without showing output

(MD014, commands-show-output)

🪛 Flake8 (7.2.0)
benches/microbenchmarks/sort.py

[error] 4-4: undefined name 'ITERATIONS'

(F821)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run tests under miri
  • GitHub Check: Ensure compilation on various targets
🔇 Additional comments (1)
benches/microbenchmarks/sort.py (1)

8-9: LGTM: benchmarked code avoids in-place mutation

Using sorted(unsorted_list) respects the “setup runs once” constraint and keeps input unchanged across timed runs.

@youknowone
Copy link
Member

@jackoconnordev your email address of the commits and your github id seems different. could you check your local setting by running git config user.email?

Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! please check the file name and the local git settings

I chose 5 * Iterations to try better show that RustPython
sort implementation scales noticeably worse CPython's
with respect to the number of elements.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (4)
benches/README.md (3)

10-16: Clarify filter name and fix markdownlint MD014 ($ prompt) in command example.

  • "sort microbenchmark" vs file name "sorted.py" can confuse readers. Using the exact name avoids accidental matches with other benches.
  • Remove leading "$" to satisfy MD014.

Apply this diff:

-`cargo bench` supports name matching to run a subset of the benchmarks. To
-run only the sort microbenchmark, you can run:
+`cargo bench` supports name matching to run a subset of the benchmarks. To
+run only the sorted microbenchmark, run:
 
 ```shell
-$ cargo bench sort
+cargo bench sorted

---

`17-22`: **Prefer “benchmark” over “bench” and fix MD014 ($ prompt) in the Python example.**

- Minor wording polish.
- Remove leading "$" for markdownlint.


Apply this diff:

```diff
-To view Python tracebacks during benchmarks, run `RUST_BACKTRACE=1 cargo bench`. You can also bench against a
+To view Python tracebacks during benchmarks, run `RUST_BACKTRACE=1 cargo bench`. You can also benchmark against a
 specific installed Python version by running:
 
 ```shell
-$ PYTHON_SYS_EXECUTABLE=python3.13 cargo bench
+PYTHON_SYS_EXECUTABLE=/usr/bin/python3.13 cargo bench

---

`7-8`: **Fix small typo (“use use”).**

Minor duplication in the sentence.


Apply this diff:

```diff
-Running `cargo bench` from the root of the repository will start the benchmarks. Once done there will be a graphical 
-report under `target/criterion/report/index.html` that you can use use to view the results.
+Running `cargo bench` from the root of the repository will start the benchmarks. Once done there will be a graphical 
+report under `target/criterion/report/index.html` that you can use to view the results.
benches/microbenchmarks/sorted.py (1)

1-5: Avoid F821 (undefined name) and make the file runnable outside the harness.

ITERATIONS is provided by the harness at runtime, but static linters (ruff/flake8 F821) and manual runs will flag/break. Add a harmless fallback that the harness will override.

Apply this diff:

 from random import random, seed
 seed(0)
 
+# Fallback for linters/manual runs; the harness sets ITERATIONS before execution.
+if "ITERATIONS" not in globals():
+    ITERATIONS = 1000
+
 unsorted_list = [random() for _ in range(5 * ITERATIONS)]
📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between fb3dd8f and 783d22e.

📒 Files selected for processing (2)
  • benches/README.md (1 hunks)
  • benches/microbenchmarks/sorted.py (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.py: Follow PEP 8 style for custom Python code
Use ruff for linting Python code

Files:

  • benches/microbenchmarks/sorted.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.512Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.
📚 Learning: 2025-08-09T22:55:10.512Z
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.512Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.

Applied to files:

  • benches/microbenchmarks/sorted.py
  • benches/README.md
🧬 Code graph analysis (1)
benches/microbenchmarks/sorted.py (2)
stdlib/src/random.rs (2)
  • random (42-45)
  • seed (48-67)
vm/src/stdlib/builtins.rs (1)
  • sorted (815-820)
🪛 Flake8 (7.2.0)
benches/microbenchmarks/sorted.py

[error] 4-4: undefined name 'ITERATIONS'

(F821)

🪛 LanguageTool
benches/README.md

[grammar] ~10-~10: There might be a mistake here.
Context: ...ng to run a subset of the benchmarks. To run only the sort microbenchmark, you ca...

(QB_NEW_EN)


[grammar] ~17-~17: There might be a mistake here.
Context: ...rgo bench`. You can also bench against a specific installed Python version by run...

(QB_NEW_EN)

🪛 markdownlint-cli2 (0.17.2)
benches/README.md

14-14: Dollar signs used before commands without showing output

(MD014, commands-show-output)


21-21: Dollar signs used before commands without showing output

(MD014, commands-show-output)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run tests under miri
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Ensure compilation on various targets
🔇 Additional comments (3)
benches/README.md (1)

26-27: Verify directory name: benchmarks/ vs benches/.

This README lives under benches/ and the new microbench is in benches/microbenchmarks/. The sentence says “Simply adding a file to the benchmarks/ directory…”, which might be inaccurate.

If this is indeed meant to be benches/, apply:

-Simply adding a file to the `benchmarks/` directory will add it to the set of files benchmarked. Each file is tested 
+Simply adding a file to the `benches/` directory will add it to the set of files benchmarked. Each file is tested 
benches/microbenchmarks/sorted.py (2)

1-3: Deterministic setup is great.

Seeding the RNG makes the dataset stable across runs and implementations. Good call.


8-9: LGTM: Benchmark code does not mutate setup state.

Using sorted(unsorted_list) preserves the original list between runs, matching the intent noted in the comment.

Jack O'Connor added 2 commits August 27, 2025 08:53
3.13 better reflects the current state of the project vs 3.7.
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (6)
benches/README.md (6)

7-8: Fix typo and readability in benchmark report sentence.

Remove the duplicated word and add a comma for flow.

-Running `cargo bench` from the root of the repository will start the benchmarks. Once done there will be a graphical
-report under `target/criterion/report/index.html` that you can use use to view the results.
+Running `cargo bench` from the root of the repository will start the benchmarks. Once done, there will be a graphical
+report under `target/criterion/report/index.html` that you can use to view the results.

10-15: Clarify how name matching works.

Note that the filter is substring-based and may match multiple benchmarks.

-`cargo bench` supports name matching to run a subset of the benchmarks. To
-run only the sorted microbenchmark, you can run:
+`cargo bench` supports substring-based name matching to run a subset of the benchmarks. To
+run only the sorted microbenchmark, you can run (this may also match other names containing "sorted"):

17-22: Make Python version example resilient across environments.

Keep 3.13, but note that any installed version is acceptable.

-To view Python tracebacks during benchmarks, run `RUST_BACKTRACE=1 cargo bench`. You can also bench against a
-specific installed Python version by running:
+To view Python tracebacks during benchmarks, run `RUST_BACKTRACE=1 cargo bench`. You can also bench against a
+specific installed Python version by running (adjust to your local version if different):

26-27: Disambiguate benchmark directory path.

Readers at repo root may misread this as a top-level path.

-Simply adding a file to the `benchmarks/` directory will add it to the set of files benchmarked. Each file is tested
+Simply adding a file to the `benches/benchmarks/` directory will add it to the set of files benchmarked. Each file is tested

34-36: Standardize terminology: “microbenchmark” (closed form).

Use a single, consistent term.

-Micro benchmarks are small snippets of code added under the `microbenchmarks/` directory. A microbenchmark file has
+Microbenchmarks are small snippets of code added under the `microbenchmarks/` directory. A microbenchmark file has

Also consider updating the section header on Line 32 to “Adding a microbenchmark” for consistency.


60-60: Use Apple’s casing: “macOS”.

-## MacOS setup
+## macOS setup
📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 783d22e and 064a0e0.

📒 Files selected for processing (1)
  • benches/README.md (4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:7-7
Timestamp: 2025-08-09T22:55:10.512Z
Learning: In RustPython benchmarks, when testing sorting performance, benchmarking only `sorted()` is sufficient since it internally uses the same sorting logic as `list.sort()`. The only difference is that `sorted()` creates a copy first, so testing both would be redundant for evaluating the sorting algorithm's performance.
📚 Learning: 2025-08-09T22:56:24.527Z
Learnt from: jackoconnordev
PR: RustPython/RustPython#6086
File: benches/microbenchmarks/sort.py:1-3
Timestamp: 2025-08-09T22:56:24.527Z
Learning: In RustPython's microbenchmarks (benches/microbenchmarks/*.py), the variable `ITERATIONS` is intentionally used without being defined in the Python files. It is injected by the cargo bench harness at runtime. This pattern should be maintained for consistency across all microbenchmarks, and F821 lint warnings for undefined `ITERATIONS` are expected and acceptable in this context.

Applied to files:

  • benches/README.md
🪛 LanguageTool
benches/README.md

[grammar] ~7-~7: There might be a mistake here.
Context: ...rks. Once done there will be a graphical report under `target/criterion/report/in...

(QB_NEW_EN)


[grammar] ~8-~8: There might be a mistake here.
Context: ...ion/report/index.htmlthat you can use use to view the results. cargo bench` sup...

(QB_NEW_EN)


[grammar] ~10-~10: There might be a mistake here.
Context: ...ng to run a subset of the benchmarks. To run only the sorted microbenchmark, you ...

(QB_NEW_EN)


[grammar] ~17-~17: There might be a mistake here.
Context: ...rgo bench`. You can also bench against a specific installed Python version by run...

(QB_NEW_EN)


[grammar] ~26-~26: There might be a mistake here.
Context: ...f files benchmarked. Each file is tested in two ways: 1. The time to parse the f...

(QB_NEW_EN)


[grammar] ~29-~29: There might be a mistake here.
Context: ...s: 1. The time to parse the file to AST 2. The time it takes to execute the file #...

(QB_NEW_EN)


[grammar] ~34-~34: There might be a mistake here.
Context: ...s/` directory. A microbenchmark file has two sections: 1. Optional setup code 2....

(QB_NEW_EN)


[grammar] ~50-~50: There might be a mistake here.
Context: ...rence a variable called ITERATIONS. If present then the benchmark code will be ...

(QB_NEW_EN)


[grammar] ~51-~51: There might be a mistake here.
Context: ...et to a value between 100 and 1,000. For example: ```python obj = [i for i in ra...

(QB_NEW_EN)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Check the WASM package and demo
  • GitHub Check: Check Rust code with rustfmt and clippy
  • GitHub Check: Run snippets and cpython tests (windows-latest)
  • GitHub Check: Run snippets and cpython tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests (ubuntu-latest)
  • GitHub Check: Run rust tests (ubuntu-latest)
  • GitHub Check: Run tests under miri
  • GitHub Check: Run rust tests (windows-latest)
  • GitHub Check: Run rust tests (macos-latest)
  • GitHub Check: Run snippets and cpython tests on wasm-wasi
  • GitHub Check: Ensure compilation on various targets
🔇 Additional comments (1)
benches/README.md (1)

74-74: LGTM: link formatting improvement.

Angle-bracket link renders well across Markdown engines.

Comment on lines +50 to 52
Only `len(a_list)` will be timed. Setup or benchmarked code can optionally reference a variable called `ITERATIONS`. If
present then the benchmark code will be invoked 5 times with `ITERATIONS` set to a value between 100 and 1,000. For
example:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Verification agent

🧩 Analysis chain

Verify accuracy: does the harness invoke benchmarks exactly 5 times?

The microbenchmark example uses 5 * ITERATIONS in its own code, but the harness’s number of invocations may differ. Past learnings note ITERATIONS is injected at runtime, not the exact repeat count. Please confirm before locking this to “5 times.” If uncertain, prefer “multiple times.”

Proposed wording if needed:

-Only `len(a_list)` will be timed. Setup or benchmarked code can optionally reference a variable called `ITERATIONS`. If
-present then the benchmark code will be invoked 5 times with `ITERATIONS` set to a value between 100 and 1,000. For
+Only `len(a_list)` will be timed. Setup or benchmarked code can optionally reference a variable called `ITERATIONS`. If
+present, then the benchmark code will be invoked multiple times with `ITERATIONS` set to a value between 100 and 1,000. For

Run this script to locate the harness logic that sets ITERATIONS and confirm the count/values:


🏁 Script executed:

#!/bin/bash
# Find where ITERATIONS is injected and how many distinct values are used.
rg -nC3 -S '\bITERATIONS\b' benches | sed -n '1,200p'
rg -nC2 -S 'microbenchmark|microbenchmarks' benches

Length of output: 9032


Clarify README: harness invokes benchmarks multiple times, not exactly five

Verified by inspecting benches/microbenchmarks.rs that there is no hard-coded “5” invocations—ITERATIONS is injected for each run based on Criterion’s input values, and the harness loops over a range of iteration counts rather than invoking exactly five times.

Please update benches/README.md accordingly:

-Only `len(a_list)` will be timed. Setup or benchmarked code can optionally reference a variable called `ITERATIONS`. If
-present then the benchmark code will be invoked 5 times with `ITERATIONS` set to a value between 100 and 1,000. For
+Only `len(a_list)` will be timed. Setup or benchmarked code can optionally reference a variable called `ITERATIONS`. If
+present, then the benchmark code will be invoked multiple times with `ITERATIONS` set to a value between 100 and 1,000. For

Relevant implementation details:

  • In benches/microbenchmarks.rs, the harness checks for ITERATIONS in each benchmark and injects it (lines ~65–67), but does not fix the count at five.
  • Criterion’s BenchmarkGroup then runs the benchmark for each input value, driving multiple invocations rather than exactly five.
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
Only `len(a_list)` will be timed. Setup or benchmarked code can optionally reference a variable called `ITERATIONS`. If
present then the benchmark code will be invoked 5 times with `ITERATIONS` set to a value between 100 and 1,000. For
example:
Only `len(a_list)` will be timed. Setup or benchmarked code can optionally reference a variable called `ITERATIONS`. If
present, then the benchmark code will be invoked multiple times with `ITERATIONS` set to a value between 100 and 1,000. For
example:
🧰 Tools
🪛 LanguageTool

[grammar] ~50-~50: There might be a mistake here.
Context: ...rence a variable called ITERATIONS. If present then the benchmark code will be ...

(QB_NEW_EN)


[grammar] ~51-~51: There might be a mistake here.
Context: ...et to a value between 100 and 1,000. For example: ```python obj = [i for i in ra...

(QB_NEW_EN)

🤖 Prompt for AI Agents
In benches/README.md around lines 50 to 52, the README incorrectly states the
harness invokes benchmarks exactly five times; update the text to clarify that
the harness injects ITERATIONS per run and then Criterion runs the benchmark
across a range of input values (so benchmarks are invoked multiple times driven
by those inputs rather than a fixed count of five). Mention that
benches/microbenchmarks.rs injects ITERATIONS when present (lines ~65–67) and
that BenchmarkGroup executes the benchmark for each input value, and adjust the
example wording to reflect multiple runs over a range instead of “5 times.”

@youknowone youknowone changed the title Add sort.py to microbenchmarks Add sorted.py to microbenchmarks Aug 28, 2025
Copy link
Member

@youknowone youknowone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@youknowone youknowone merged commit 763d5d4 into RustPython:main Aug 28, 2025
12 checks passed
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