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

Merged
merged 1 commit into from
Jul 29, 2025

Conversation

illusory0x0
Copy link
Contributor

No description provided.

Copy link

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

Unsafe mutation of immutable data structure without proper type safety

Category
Correctness
Code Snippet
ptr_.tail = More(x, tail=Empty)
ptr = ptr_.tail
Recommendation
Use a mutable reference or builder pattern instead of mutating the tail field directly, or ensure the List type properly supports this mutation pattern
Reasoning
Direct mutation of the tail field could lead to memory safety issues or inconsistent state if the List type is designed to be immutable. The pattern suggests treating an immutable structure as mutable.

Complex state management with manual pointer tracking

Category
Maintainability
Code Snippet
let mut res = Empty
let mut ptr = Empty
for x in iter {
match (res, ptr) {
(Empty, ) => {
res = More(x, tail=Empty)
ptr = res
}
(More(
), More() as ptr) => {
ptr_.tail = More(x, tail=Empty)
ptr = ptr_.tail
}
Recommendation
Consider using a helper function or builder pattern to encapsulate the list construction logic, making it more readable and less error-prone
Reasoning
The current implementation requires careful tracking of two mutable variables and complex pattern matching, making it harder to understand and maintain compared to the simpler fold+reverse approach.

Unreachable panic case may indicate incomplete pattern matching

Category
Correctness
Code Snippet
(_, _) => panic()
Recommendation
Either remove the panic case if it's truly unreachable, or add a comment explaining why this case should never occur, or handle it more gracefully
Reasoning
The catch-all panic suggests either incomplete understanding of possible states or a design flaw. If this case is truly impossible, it should be documented why, or the pattern matching should be more specific.

@coveralls
Copy link
Collaborator

coveralls commented Apr 29, 2025

Pull Request Test Coverage Report for Build 573

Details

  • 4 of 5 (80.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.007%) to 90.108%

Changes Missing Coverage Covered Lines Changed/Added Lines %
list/list.mbt 4 5 80.0%
Totals Coverage Status
Change from base Build 572: -0.007%
Covered Lines: 8472
Relevant Lines: 9402

💛 - Coveralls

@bobzhang bobzhang force-pushed the perf-list-from_iter branch from 1d7b264 to 46f7cdf Compare May 1, 2025 04:22
@illusory0x0 illusory0x0 force-pushed the perf-list-from_iter branch from 46f7cdf to 4f6022e Compare May 15, 2025 07:58
@peter-jerry-ye
Copy link
Collaborator

How's the performance comparison?

@illusory0x0
Copy link
Contributor Author

one pass implementation is a litter faster than original one.

bench result

illu@illusory0x0 ~/d/b/moon-bench (master)> moon bench --target wasm,native
bench username/hello/top.mbt::0
name time (mean ± σ)         range (min … max) 
v1   45.83 µs ±   1.32 µs    45.11 µs …  49.47 µs  in 10 ×   2196 runs
v2   49.36 µs ±   0.28 µs    49.06 µs …  49.95 µs  in 10 ×   2037 runs
Total tests: 1, passed: 1, failed: 0. [wasm]
bench username/hello/top.mbt::0
name time (mean ± σ)         range (min … max) 
v1   27.75 µs ±   0.17 µs    27.60 µs …  28.12 µs  in 10 ×   3636 runs
v2   28.79 µs ±   0.49 µs    28.23 µs …  29.56 µs  in 10 ×   3402 runs
Total tests: 1, passed: 1, failed: 0. [native]

Example

pub enum T[A] {
  Empty
  More(A, mut tail~ : T[A])
} derive(Eq)


pub fn from_iter_v1[A](iter : Iter[A]) -> T[A] {
  let mut res = Empty
  let mut ptr = Empty
  for x in iter {
    match (res, ptr) {
      (Empty, _) => {
        res = More(x, tail=Empty)
        ptr = res
      }
      (More(_), More(_) as ptr_) => {
        ptr_.tail = More(x, tail=Empty)
        ptr = ptr_.tail
      }
      (_, _) => abort("unreachable")
    }
  }
  res
}


pub fn from_iter_v2[A](iter : Iter[A]) -> T[A] {
  iter.fold(init=Empty, fn(acc, e) { More(e, tail=acc) }).reverse_inplace()

}

fn reverse_inplace[A](self : T[A]) -> T[A] {
  match self {
    Empty | More(_, tail=Empty) => self
    More(head, tail~) =>
      loop More(head, tail=Empty), tail {
        result, Empty => result
        result, More(_, tail=xs) as new_result => {
          new_result.tail = result
          continue new_result, xs
        }
      }
  }
}


test (b : @bench.T) {
  let xs = Int::until(0,1000)

  b.bench(name="v1",fn() { b.keep(from_iter_v1(xs)) })
  b.bench(name="v2",fn() { b.keep(from_iter_v2(xs)) })
}

@illusory0x0 illusory0x0 force-pushed the perf-list-from_iter branch from b59e88b to 869e957 Compare May 30, 2025 08:11
@peter-jerry-ye
Copy link
Collaborator

It looks drastically faster on wasm-gc. Seems to be a reasonable change.

@illusory0x0
Copy link
Contributor Author

It looks drastically faster on wasm-gc. Seems to be a reasonable change.

benchmark result is unstable in js backend and wasm-gc backend due to GC.

moonbitlang/moon#813

@illusory0x0
Copy link
Contributor Author

@peter-jerry-ye Should we close this PR since this change is a weak performance boost?

@peter-jerry-ye
Copy link
Collaborator

I'm fine with both implementation. If you are ready you can mark it as 'Ready for review'.

@illusory0x0 illusory0x0 force-pushed the perf-list-from_iter branch from 869e957 to bf74a4c Compare July 23, 2025 05:11
@illusory0x0 illusory0x0 marked this pull request as ready for review July 23, 2025 05:13
@bobzhang bobzhang force-pushed the perf-list-from_iter branch from bf74a4c to 6d8a9a4 Compare July 29, 2025 02:17
@bobzhang bobzhang enabled auto-merge (rebase) July 29, 2025 02:19
@bobzhang bobzhang merged commit a6800cb into moonbitlang:main Jul 29, 2025
11 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.

4 participants