Skip to content
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

malloc: optimizations (#16037) #16039

Merged
merged 3 commits into from May 13, 2024
Merged

malloc: optimizations (#16037) #16039

merged 3 commits into from May 13, 2024

Conversation

reusee
Copy link
Contributor

@reusee reusee commented May 11, 2024

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #15479

What this PR does / why we need it:

malloc: refine cache size calculation

malloc: remove AllocTyped

malloc: remove eviction

malloc: add shard alloc and free statistics

malloc: flush cached objects when idle

malloc: refine cache size calculation

malloc: remove AllocTyped

malloc: remove eviction

malloc: add shard alloc and free statistics

malloc: flush cached objects when idle

Approved by: @zhangxu19830126
@matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label May 11, 2024
@mergify mergify bot requested a review from sukki37 May 11, 2024 15:49
@matrix-meow
Copy link
Contributor

@reusee Thanks for your contributions!

Pull Request Review:

Title: malloc: optimizations (#16037)

Body:

  • The PR is categorized as an Improvement.
  • It addresses issue [Refactoring]: Remove or replace mmap with malloc #15479.
  • The changes include refining cache size calculation, removing AllocTyped, removing eviction, adding shard alloc and free statistics, and flushing cached objects when idle.

Changes:

  1. Bench_test.go:

    • Removed BenchmarkAllocTyped and BenchmarkParallelAllocTyped functions.
    • This change seems appropriate as these functions are no longer needed.
  2. Flush.go:

    • Added a new file flush.go containing a function to flush cached objects when idle.
    • The implementation looks fine and adds a necessary feature.
  3. Handle.go:

    • Modified the Free method to use a shard based on the processor ID for better distribution.
    • This change improves the distribution of objects across shards.
  4. Malloc.go:

    • Replaced some global variables with a Shard struct to manage allocation and deallocation statistics per shard.
    • Refactored the initialization of shards and pools to improve memory management.
    • Removed AllocTyped function which is no longer used.
    • The changes seem to optimize memory allocation and deallocation processes.

Suggestions for Improvement:

  1. Code Refactoring:

    • Consider breaking down the changes into smaller, more focused commits for better readability and easier review.
    • Ensure consistent formatting and naming conventions across the codebase.
  2. Error Handling:

    • Add error handling mechanisms where necessary, especially in functions that involve critical operations like memory allocation and deallocation.
  3. Documentation:

    • Provide clear and concise comments for new functions, structs, and significant code blocks to enhance code maintainability and readability.
  4. Testing:

    • Ensure that the changes are thoroughly tested to validate the performance improvements and prevent regressions.
  5. Security Considerations:

    • Review the changes for any potential security vulnerabilities, especially in memory management operations, to prevent exploitation.
  6. Performance Optimization:

    • Continuously monitor the performance impact of the optimizations introduced and fine-tune them if needed for better efficiency.

By addressing the suggestions mentioned above, the codebase can be further improved in terms of readability, maintainability, and performance optimization.

@mergify mergify bot merged commit 22194fc into matrixorigin:1.2-dev May 13, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement size/M Denotes a PR that changes [100,499] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants