Skip to content

Memory Leak in memory.c with VirtualFree Function on Windows #2370

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

Closed
hjmndv opened this issue Jan 18, 2020 · 4 comments
Closed

Memory Leak in memory.c with VirtualFree Function on Windows #2370

hjmndv opened this issue Jan 18, 2020 · 4 comments

Comments

@hjmndv
Copy link

hjmndv commented Jan 18, 2020

Hello, I would like to share a memory leak problem on Windows and a suggestion to solve it.

I am using single-threaded openblas 0.3.7 version with c++ plugin and Windows functions like #1152. A memory leak regarding Private Usage was found for each execution of set of LoadLibrary, BLAS function and FreeLibrary. The size is around 64KB per only one execution. It will be terrible if it piles up.

This occurs because a configuration parameter "MEM_DECOMMIT" of the Windows VirtualFree function is used in memory.c in OpenBLAS. According to MS documentation, VirtualFree has another parameter "MEM_RELEASE" to free memory. I have confirmed that replacing "MEM_DECOMMIT" with "MEM _ RELEASE" solves the memory leak.

The test condition is as follows:

  • OpenBLAS Version : 0.3.7

  • Change Points:

    • memory.c(825)
      //VirtualFree(alloc_info, allocation_block_size, MEM_DECOMMIT);
      VirtualFree(alloc_info, 0, MEM_RELEASE);
    • I also modified other three VirtualFree functions in memory.c like above.
  • Build Machine: Ubuntu 19.04 with MinGW-w64, gcc version 8.3.0

  • Build Option: make BINARY=64 HOSTCC=gcc CC=x86_64-w64-mingw32-gcc CFLAGS='-static-libgcc -static-libstdc++' USE_THREAD=0

  • Test Machine: Windows 8.1 Pro

  • Test Program:


#include <psapi.h> 
typedef void(*pfnc_cblas_dgemm)(OPENBLAS_CONST enum CBLAS_ORDER Order, OPENBLAS_CONST enum CBLAS_TRANSPOSE TransA, OPENBLAS_CONST enum, CBLAS_TRANSPOSE TransB, OPENBLAS_CONST blasint M, OPENBLAS_CONST blasint N, OPENBLAS_CONST blasint K, OPENBLAS_CONST double alpha, OPENBLAS_CONST double *A, OPENBLAS_CONST blasint lda, OPENBLAS_CONST double *B, OPENBLAS_CONST blasint ldb, OPENBLAS_CONST double beta, double *C, OPENBLAS_CONST blasint ldc);

int main(){
    // definition of some values

    for (int i = 0; i < 10000; i++) {
        // LoadLibrary, dgemm and FreeLibrary
        HMODULE dll = LoadLibrary(L"libopenblas.dll");
        FARPROC proc = GetProcAddress(dll, "cblas_dgemm");
        pfnc_cblas_dgemm dgemm = reinterpret_cast<pfnc_cblas_dgemm>(proc);
        dgemm(CblasRowMajor, CblasNoTrans, CblasNoTrans, L, N, M, 1.0, A, M, B, N, 0.0, C, N);
        FreeLibrary(dll);

        // Get Private Usage information
        HANDLE hProc = GetCurrentProcess();
        PROCESS_MEMORY_COUNTERS_EX info;
        GetProcessMemoryInfo(hProc, (PROCESS_MEMORY_COUNTERS*)&info, sizeof(info));
        printf("Private Usage %d: %zd\n", i, info.PrivateUsage); //After the modification, PrivateUsage does not change.
    }
}

I would recommend using "MEM_RELASE" parameter to solve the memory leak problem.
Could you please fix it in the next release?

Thank you!

@brada4
Copy link
Contributor

brada4 commented Jan 18, 2020

It does not maintain physical memory for decommited address space.

@martin-frbg
Copy link
Collaborator

I have not found a clear definition yet, but it seems it would indeed leak virtual memory, which could be bad enough to eventually cause serious problems. (One similar case was discussed on the cygwin mailing list years ago, the pertinent part starts at http://www.cygwin.net/ml/cygwin-developers/2003-04/msg00032.html)

@martin-frbg
Copy link
Collaborator

The suggested fix was committed as PR #2371

@hjmndv
Copy link
Author

hjmndv commented Jan 20, 2020

Thank you so much for the update!

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

No branches or pull requests

3 participants