Skip to content

Memory limit tuning #166

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 3 commits into from
Apr 22, 2025
Merged

Memory limit tuning #166

merged 3 commits into from
Apr 22, 2025

Conversation

misrasaurabh1
Copy link
Contributor

@misrasaurabh1 misrasaurabh1 commented Apr 22, 2025

User description

disables total memory check for mac - which are mostly dev laptops and have dynamic swaps
for linux sets the total memory limit as 85% of main memory + swap


PR Type

Enhancement


Description

  • Restrict memory limit to Linux only

  • Include swap in memory calculation

  • Increase limit factor to 85%

  • Compute swap size dynamically


Changes walkthrough 📝

Relevant files
Enhancement
pytest_plugin.py
Enhance Linux memory limit calculation                                     

codeflash/verification/pytest_plugin.py

  • Added swap detection from /proc/swaps
  • Summed swap_size into total_memory
  • Updated memory limit factor to 85%
  • Removed macOS/Darwin branch for limits
  • +31/-5   

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Conditional Limiter

    Verify that resource.setrlimit only runs on Linux and does not cause a NameError on other platforms since resource is imported inside a Linux-only block.

    if platform.system() == "Linux":
        import resource
    
        # We set the memory limit to 85% of total system memory + swap when swap exists
        swap_file_path = Path("/proc/swaps")
        swap_exists = swap_file_path.is_file()
        swap_size = 0
    
        if swap_exists:
            with swap_file_path.open("r") as f:
                swap_lines = f.readlines()
                swap_exists = len(swap_lines) > 1  # First line is header
    
                if swap_exists:
                    # Parse swap size from lines after header
                    for line in swap_lines[1:]:
                        parts = line.split()
                        if len(parts) >= 3:
                            # Swap size is in KB in the 3rd column
                            try:
                                swap_size += int(parts[2]) * 1024  # Convert KB to bytes
                            except (ValueError, IndexError):
                                pass
    
        # Get total system memory
        total_memory = os.sysconf("SC_PAGE_SIZE") * os.sysconf("SC_PHYS_PAGES")
    
        # Add swap to total available memory if swap exists
        if swap_exists:
            total_memory += swap_size
    
        # Set the memory limit to 85% of total memory (RAM plus swap)
        memory_limit = int(total_memory * 0.85)
    
        # Set both soft and hard limits
        resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    Swap Parsing Logic

    Ensure the logic for detecting and summing swap entries handles edge cases correctly (e.g., no swap devices or malformed lines) and that swap_exists reflects actual swap availability.

    swap_file_path = Path("/proc/swaps")
    swap_exists = swap_file_path.is_file()
    swap_size = 0
    
    if swap_exists:
        with swap_file_path.open("r") as f:
            swap_lines = f.readlines()
            swap_exists = len(swap_lines) > 1  # First line is header
    
            if swap_exists:
                # Parse swap size from lines after header
                for line in swap_lines[1:]:
                    parts = line.split()
                    if len(parts) >= 3:
                        # Swap size is in KB in the 3rd column
                        try:
                            swap_size += int(parts[2]) * 1024  # Convert KB to bytes
                        except (ValueError, IndexError):
                            pass
    Import-Time Overhead

    Consider whether reading /proc/swaps and computing memory limits at import time may introduce unwanted startup delays and if lazy evaluation or caching is preferable.

    swap_file_path = Path("/proc/swaps")
    swap_exists = swap_file_path.is_file()
    swap_size = 0
    
    if swap_exists:
        with swap_file_path.open("r") as f:
            swap_lines = f.readlines()
            swap_exists = len(swap_lines) > 1  # First line is header
    
            if swap_exists:
                # Parse swap size from lines after header
                for line in swap_lines[1:]:
                    parts = line.split()
                    if len(parts) >= 3:
                        # Swap size is in KB in the 3rd column
                        try:
                            swap_size += int(parts[2]) * 1024  # Convert KB to bytes
                        except (ValueError, IndexError):
                            pass
    
    # Get total system memory
    total_memory = os.sysconf("SC_PAGE_SIZE") * os.sysconf("SC_PHYS_PAGES")
    
    # Add swap to total available memory if swap exists
    if swap_exists:
        total_memory += swap_size
    
    # Set the memory limit to 85% of total memory (RAM plus swap)
    memory_limit = int(total_memory * 0.85)

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Clamp memory limit to hard limit

    Clamp the new soft limit to the existing hard limit by querying getrlimit before
    calling setrlimit to avoid exceeding allowed bounds.

    codeflash/verification/pytest_plugin.py [76]

    -resource.setrlimit(resource.RLIMIT_AS, (memory_limit, memory_limit))
    +soft, hard = resource.getrlimit(resource.RLIMIT_AS)
    +limit = min(memory_limit, hard)
    +resource.setrlimit(resource.RLIMIT_AS, (limit, limit))
    Suggestion importance[1-10]: 7

    __

    Why: Querying the existing hard limit and clamping the new soft limit avoids setrlimit errors when memory_limit exceeds allowed bounds.

    Medium
    Handle swap file read errors

    Wrap reading /proc/swaps in a try-except to gracefully handle I/O errors and avoid
    crashing on permission issues.

    codeflash/verification/pytest_plugin.py [50-52]

    -with swap_file_path.open("r") as f:
    -    swap_lines = f.readlines()
    -    swap_exists = len(swap_lines) > 1  # First line is header
    +try:
    +    with swap_file_path.open("r") as f:
    +        swap_lines = f.readlines()
    +        swap_exists = len(swap_lines) > 1  # First line is header
    +        ...
    +except OSError:
    +    swap_size = 0
    +    swap_exists = False
    Suggestion importance[1-10]: 6

    __

    Why: Wrapping the swap file read in a try‑except block enhances robustness by preventing crashes on I/O or permission failures.

    Low
    General
    Separate swap status flags

    Use distinct flags for swap file existence and swap entry presence instead of
    reusing swap_exists for both.

    codeflash/verification/pytest_plugin.py [45-46]

     swap_file_path = Path("/proc/swaps")
    -swap_exists = swap_file_path.is_file()
    +swap_file_exists = swap_file_path.is_file()
    +has_swap_entries = False
    +swap_size = 0
    Suggestion importance[1-10]: 5

    __

    Why: Introducing distinct flags for file existence and swap entries improves readability and prevents logic errors from reusing swap_exists for two different checks.

    Low

    @misrasaurabh1 misrasaurabh1 requested a review from KRRT7 April 22, 2025 06:09
    @KRRT7 KRRT7 merged commit 9f967a7 into main Apr 22, 2025
    16 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants