Skip to content

Conversation

peter-jerry-ye
Copy link
Collaborator

Replaces #2210

Copy link

Unnecessary capacity allocation for empty maps

Category
Performance
Code Snippet
if self.is_empty() { return Map::new() }
Recommendation
Remove the empty check and let the capacity initialization handle both cases:

let m = Map::new(capacity=self.size)

Reasoning
The special case for empty maps creates an unnecessary branch and allocates a map without capacity hint. Since Map::new() with capacity 0 should handle empty maps efficiently, we can simplify the code and potentially improve performance.

Inconsistent iteration pattern usage

Category
Maintainability
Code Snippet
for e in self.iter() { ... } // in filter
for e in self { ... } // in fold
Recommendation
Standardize on using either direct iteration or iter() method:

for e in self { ... }

Reasoning
Using different iteration patterns for the same data structure makes the code less consistent and harder to maintain. Direct iteration is more idiomatic and cleaner when no special iterator features are needed.

Documentation could be more detailed about performance characteristics

Category
Maintainability
Code Snippet
///|
/// Folds the map from an initial value by applying a function to each key-value pair and the accumulator.
/// The order of folding is not guaranteed.
Recommendation
Add complexity information and usage examples:

///|
/// Folds the map from an initial value by applying a function to each key-value pair and the accumulator.
/// The order of folding is not guaranteed.
/// Time complexity: O(n) where n is the size of the map.
/// Example:
/// ```
/// let sum = map.fold(0, (acc, k, v) -> acc + v)
/// ```
**Reasoning**
More detailed documentation helps users understand the performance implications and proper usage of the functions, leading to better maintainability and fewer potential misuses.

</details>

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7144

Details

  • 0 of 14 (0.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 92.993%

Changes Missing Coverage Covered Lines Changed/Added Lines %
builtin/linked_hash_map.mbt 0 14 0.0%
Totals Coverage Status
Change from base Build 7141: -0.1%
Covered Lines: 8574
Relevant Lines: 9220

💛 - Coveralls

@peter-jerry-ye
Copy link
Collaborator Author

We are closing this PR for now as this does not provide any benefit (efficiency) compared with using an external implementation. We have limited bandwidth so we are not adding this for now. We'll come back to it when we have more time.

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