Skip to content

perf: improve @list.from_iter use one pass #2036

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

illusory0x0
Copy link
Contributor

No description provided.

Copy link

peter-jerry-ye-code-review bot commented Apr 29, 2025

The new implementation avoids double traversal by building the list in correct order

Category
Performance
Code Snippet
Old: iter.fold(init=Empty, fn(acc, e) { More(e, tail=acc) }).reverse_inplace()
New: Direct forward construction with ptr tracking
Recommendation
The new implementation is actually better for performance since it avoids the need for reverse_inplace(). Keep the new change.
Reasoning
The old implementation first built the list in reverse order using fold, then had to reverse it again with reverse_inplace(). The new implementation builds the list in the correct order in a single pass.

The ptr pattern match could be simplified and made more robust

Category
Correctness
Code Snippet
match (res, ptr) {
(Empty, ) => {...}
(More(
), More() as ptr) => {...}
(_, _) => abort("unreachable")
}
Recommendation
Consider simplifying to only match on res:

if res == Empty {
  res = More(x, tail=Empty)
  ptr = res
} else {
  match ptr {
    More(_, _) as ptr_ => {
      ptr_.tail = More(x, tail=Empty)
      ptr = ptr_.tail
    }
    _ => abort("unreachable")
  }
}```
**Reasoning**
The current pattern matching is more complex than needed and includes cases that should truly be unreachable. The simplified version makes the control flow clearer.

</details>
<details>

<summary> Consider adding inline comments explaining the pointer manipulation </summary>

**Category**
Maintainability
**Code Snippet**
let mut ptr = Empty
// ... ptr manipulation in loop
**Recommendation**
Add comments explaining the pointer tracking:
```moonbit
// ptr tracks the last node for efficient append
let mut ptr = Empty
// ... rest of code```
**Reasoning**
While the code is functionally correct, the use of a trailing pointer for list construction is a pattern that deserves explanation for future maintainers.

</details>

@coveralls
Copy link
Collaborator

coveralls commented Apr 29, 2025

Pull Request Test Coverage Report for Build 6555

Details

  • 3 of 4 (75.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.01%) to 92.5%

Changes Missing Coverage Covered Lines Changed/Added Lines %
list/list.mbt 3 4 75.0%
Totals Coverage Status
Change from base Build 6554: -0.01%
Covered Lines: 6117
Relevant Lines: 6613

💛 - Coveralls

@bobzhang bobzhang force-pushed the perf-list-from_iter branch from 1d7b264 to 46f7cdf Compare May 1, 2025 04:22
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