Skip to content

Conversation

@tony-davis
Copy link
Contributor

@tony-davis tony-davis commented Jan 6, 2026

Summary

Improved memory management in host_alloc.cpp by refactoring the deallocation logic for better efficiency and adding diagnostic warnings for untracked pointer frees.

Changes

  1. Refactored free_ptr_use() to use iterator-based access (reduces redundant map lookups)
  2. Added warning for freeing untracked pointers (helps detect potential memory corruption issues)

Motivation

The current implementation of free_ptr_use() in host_alloc.cpp had two issues:

  1. Performance: Redundant map lookups when deallocating tracked memory
  2. Correctness: Using operator[] on a map can unintentionally insert entries for non-existent keys
  3. Diagnostics: Silent failures when attempting to free untracked pointers made debugging difficult

This PR addresses all three issues by refactoring the lookup logic and adding diagnostic warnings.

Technical Details

File: projects/rocblas/clients/common/host_alloc.cpp

Change 1: Iterator-based memory deallocation

Before:

if(ptr && mem_allocated[ptr]) {           // Lookup #1 (may insert!)
    mem_used -= mem_allocated[ptr];       // Lookup #2
    mem_allocated.erase(ptr);             // Lookup #3
}

After:

auto it = mem_allocated.find(ptr);        // Single lookup
if(ptr && it != mem_allocated.end()) {
    mem_used -= it->second;               // Use cached iterator
    mem_allocated.erase(it);              // Use cached iterator
}

Benefits:

  • Reduces map lookups from 3 to 1
  • Prevents unintended map insertions via operator[]
  • More efficient memory tracking

Change 2: Diagnostic warning for untracked pointers

Added:

else if(ptr && call_free)
{
    rocblas_cerr << "Warning: Freeing untracked pointer " << ptr
                 << " - untracked memory released (potential double-free or memory corruption)"
                 << std::endl;
}

Benefits:

  • Helps detect double-free bugs
  • Identifies potential memory corruption issues
  • Provides actionable diagnostic information during testing

Test Plan

  • Built rocBLAS with no compilation errors
  • Existing client test suite exercises memory allocation/deallocation paths
  • No functional behavior changes to normal operation
  • Warning messages help identify issues during debugging

Test Result

  • All existing tests continue to pass
  • Memory tracking functionality unchanged
  • Warning properly triggers for untracked pointer frees
  • No behavioral differences for correctly tracked memory

Submission Checklist

Updated the memory deallocation logic in free_ptr_use to use an iterator for accessing allocated memory. This change improves code clarity and efficiency by directly checking the existence of the pointer in the mem_allocated map and adjusting the memory usage accordingly.
Copilot AI review requested due to automatic review settings January 6, 2026 23:55
@tony-davis tony-davis requested a review from a team as a code owner January 6, 2026 23:55
@tony-davis tony-davis changed the title [rocblas] Refactor memory management in host_alloc.cpp [rocBLAS] Refactor memory management in host_alloc.cpp Jan 6, 2026
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the memory deallocation logic in free_ptr_use() to use iterator-based map access, improving both efficiency and correctness. The change eliminates redundant map lookups and prevents unintended insertions that could occur with operator[].

Key Changes:

  • Replaced three sequential map lookups with a single find() call using an iterator
  • Used the cached iterator for subsequent operations (accessing size and erasing entry)
  • Eliminated risk of unintended map insertions when checking for pointer existence

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov-commenter
Copy link

codecov-commenter commented Jan 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3672      +/-   ##
===========================================
- Coverage    49.69%   46.04%   -3.65%     
===========================================
  Files          134      365     +231     
  Lines        33491    51594   +18103     
  Branches      4399     5964    +1565     
===========================================
+ Hits         16641    23752    +7111     
- Misses       15607    24359    +8752     
- Partials      1243     3483    +2240     
Flag Coverage Δ
hipFFT ?
rocBLAS 46.04% <ø> (?)
rocFFT ?

Flags with carried forward coverage won't be shown. Click here to find out more.
see 499 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@TorreZuk TorreZuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve but my suggestions I usually want considered to be made before merge :-)
My style is I only don't approve if I think the merge would cause trouble as is.

@TorreZuk
Copy link
Contributor

TorreZuk commented Jan 8, 2026

Looks like you have to run clang format again, otherwise only baseline fails.

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.

4 participants