8000 Optimize loops by caching length by bobzhang · Pull Request #2262 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Optimize loops by caching length #2262

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 13, 2025
Merged

Optimize loops by caching length #2262

merged 1 commit into from
Jun 13, 2025

Conversation

bobzhang
Copy link
Contributor

Summary

  • optimize loops in Bytes::to_array and Bytes addition
  • reduce length() calls in array reversal functions
  • cache size in Deque::to_array

Testing

  • moon fmt
  • moon info
  • moon check --verbose
  • moon test

https://chatgpt.com/codex/tasks/task_e_684ae1d2a7d483209a8fab3f9a02c07b

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
Multiple length() calls in loops created unnecessary overhead

Category
Performance
Code Snippet
Original code:
for i in 0..<(self.length() / 2) {
self.unsafe_set(i, self.unsafe_get(self.length() - i - 1))
Recommendation
Cache the length as implemented:
let len = self.length()
for i in 0..<(len / 2) {
self.unsafe_set(i, self.unsafe_get(len - i - 1))
Reasoning
Calling length() repeatedly in a loop is inefficient since the length doesn't change during iteration. Caching it in a local variable reduces function calls and improves performance.

Consistent approach needed for size-based initialization

Category
Maintainability
Code Snippet
In Bytes::to_array:
let rv = Array::make(self.length(), b'0')
In Deque::to_array:
let xs = Array::make(len, self[0])
Recommendation
Consider standardizing the initialization approach across similar conversion functions - either use a default value like b'0' or the first element consistently
Reasoning
Different initialization strategies in similar conversion functions can be confusing and may lead to subtle bugs. A consistent approach improves code maintainability and reduces cognitive load.

Empty deque handling could be more explicit

Category
Correctness
Code Snippet
if len == 0 {
[]
} else {
let xs = Array::make(len, self[0])
...
}
Recommendation
Add explicit documentation about empty deque handling and consider using Array::make_uninit() for empty case consistency:
if len == 0 {
Array::make_uninit(0)
}
Reasoning
The current implementation assumes self[0] is safe to access when len > 0, but this assumption should be documented. Using make_uninit() would be more consistent with other array operations in the codebase.

@bobzhang bobzhang merged commit 149d2c9 into main Jun 13, 2025
14 of 16 checks passed
@bobzhang bobzhang deleted the codex/find-performance-improvement-opportunities branch June 13, 2025 01:36
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7249

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 10 of 13 (76.92%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on codex/find-performance-improvement-opportunities at 93.109%
Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/array.mbt 3 6 50.0%
Totals Coverage Status
Change from base Build 7241: 93.1%
Covered Lines: 8567
Relevant Lines: 9201
💛 - Coveralls

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
codex
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bobzhang @coveralls
0