8000 Improve retain performance by bobzhang · Pull Request #2266 · moonbitlang/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Improve retain performance #2266

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

Improve retain performance #2266

merged 1 commit into from
Jun 13, 2025

Conversation

bobzhang
Copy link
Contributor

Summary

  • refactor Array::retain to avoid repeated length lookups and reads

Testing

  • moon fmt
  • moon info
  • moon check
  • moon test

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

@peter-jerry-ye-code-review peter-jerry-ye code review
Copy link
Good optimization by caching array length

Category
Performance
Code Snippet
let len = self.length()
for i = 0, j = 0; i < len;
Recommendation
Current implementation is good. Consider adding a comment explaining the performance benefit.
Reasoning
Caching the length avoids repeated method calls in the loop condition check. Since array length won't change during iteration, this is a safe optimization.

Good optimization by caching array element access

Category
Performance
Code Snippet
let item = self.unsafe_get(i)
if f(item) {
self.unsafe_set(j, item)
Recommendation
Current implementation is good. Consider adding a comment explaining why unsafe_get is safe in this context.
Reasoning
Caching the array element prevents multiple reads of the same index. Since we're using unsafe_get, documenting the safety guarantees would improve maintainability.

Missing documentation about the method's behavior for empty arrays or when f raises an error

Category
Maintainability
Code Snippet
pub fn[T] Array::retain(self : Array[T], f : (T) -> Bool raise?) -> Unit raise? {
Recommendation
Add documentation explaining edge cases:

/// Retains only the elements specified by the predicate.
/// If f raises an error, the array may be partially modified.
/// For empty arrays, this operation is a no-op.
pub fn[T] Array::retain(...)

Reasoning
Clear documentation about error handling and edge cases helps users understand and use the API correctly.

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7254

Details

  • 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 93.11%
Totals Coverage Status
Change from base Build 7251: 0.001%
Covered Lines: 8568
Relevant Lines: 9202
💛 - Coveralls

@bobzhang bobzhang merged commit 46d7c9e into main Jun 13, 2025
11 of 12 checks passed
@bobzhang bobzhang deleted the codex/identify-performance-improvement-opportunities branch June 13, 2025 07:36
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