[HSA] Implicitly resource leaking on kernelBufferMap

[yan-ming]

Hi @whchung,

I'm going through the HCC runtime to seek other chances to improve the overall performance for the porting applications. After some profiling experiments, I noticed that LaunchKernelWithDynamicGroupMemoryAsync somehow takes a portion of time and the cost of std::map operations seems to be one main reason for that.

By looking closer, I noticed that the map used for recording kernel-buffer dependency chain might take part here. In HSAQueue::Push(), map elements are created implicitly while there are two std::for_each calls to traverse the whole map, which takes O(map.size()) time complexity. The map will be released at a very late stage of HCC runtime.

I came up with the following patch to erase those used map elements. This does reduce the map size but I didn't actually see obvious performance gain from my application. Perhaps we might need a bigger refactoring here on the kernel-buffer dependency handling.

I knew you have another ongoing hsa_async_copy branch to leverage the AMD APIs for async copy operations, wondering if you have any further plans for async kernels here.

diff --git a/lib/hsa/mcwamp_hsa.cpp b/lib/hsa/mcwamp_hsa.cpp                    
index 7a854e8..97d943a 100644                                                   
--- a/lib/hsa/mcwamp_hsa.cpp                                                    
+++ b/lib/hsa/mcwamp_hsa.cpp                                                    
@@ -653,6 +653,7 @@ public:                                                     

         // clear data in kernelBufferMap                                       
         kernelBufferMap[ker].clear();                                          
+        kernelBufferMap.erase(ker);                                          

         delete(dispatch);                                                      
     }                                                                          
@@ -697,6 +698,7 @@ public:                                                     

         // clear data in kernelBufferMap                                       
         kernelBufferMap[ker].clear();                                          
+        kernelBufferMap.erase(ker);                                          

         return sp_dispatch;                                                    
     }

This won't bring regressions on my end.

Failing Tests (7):
    CPPAMP :: Unit/AmpShortVectors/hc_short_vector_device.cpp
    CPPAMP :: Unit/HC/memcpy_symbol1.cpp
    CPPAMP :: Unit/HC/memcpy_symbol3.cpp
    CPPAMP :: Unit/HC/wg_size.cpp
    CPPAMP :: Unit/HSAIL/shfl_xor.cpp
    CPPAMP :: Unit/SharedLibrary/shared_library2.cpp
    CPPAMP :: Unit/SharedLibrary/shared_library3.cpp

  Expected Passes    : 662
  Expected Failures  : 25
  Unsupported Tests  : 10
  Unexpected Failures: 7

[whchung]

@yan-ming in hsa_async_copy branch the work is mostly about removing hsa_memory_copy() and swithc to hsa_amd_memory_copy_async() , so it's most likely be orthogonal with your proposed changes here.

[yan-ming]

@whchung I see. Although the leaking here doesn't really hurt the performance, I'm still feeling it would be better if we can have this patch applied in HCC. Would it be okay for you if I propose a PR with the patch above to the develop branch?

[whchung]

sure thing.