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

Memory leak in service_topo.cpp on Windows x64 with core count more 32 #50

Closed
VasilyevAlexander opened this issue Aug 16, 2018 · 5 comments

Comments

@VasilyevAlexander
Copy link

VasilyevAlexander commented Aug 16, 2018

  1. On Windows x64 system in service_topo.h type LNX_PTR2INT is defined as follows:
    #ifdef _x86_64
    #define LNX_PTR2INT __int64
    #define LNX_MY1CON 1LL
    #else
    #define LNX_PTR2INT unsigned int
    #define LNX_MY1CON 1
    #endif
    Under Windows x64 macro _M_X64 should be used.
    Because LNX_PTR2INT is defined as unsigned int which on Windows is 32 bit
    then function static int __internal_daal_countBits(DWORD_PTR x) return maximum of 32,
    then functiion static void __internal_daal_setChkProcessAffinityConsistency( unsigned int lcl_OSProcessorCount )
    in statement on line 328
    if( sum != lcl_OSProcessorCount ) // check cumulative bit counts matches processor count
    determines inconsistensy;
    then in function static int __internal_daal_queryParseSubIDs(void) in statement on line 1346
    if( glbl_obj.error )
    return -1;
    error is detected which is returned to function static void __internal_daal_buildSystemTopologyTables()
    which leads to exit without initialization
  2. In function static void __internal_daal_buildSystemTopologyTables()
    tables are allocated on line 1683 and should be deallocated in case of an error
    before returns on lines: 1687, 1689, 1694
    function __internal_daal_buildSystemTopologyTables is called from
    __internal_daal_buildSystemTopologyTables which is called from
    __internal_daal_initCpuTopology which is called from
    __internal_daal_GetSysProcessorCoreCount which is called from
    GetL1CacheSize which is called in evere call to compute
@VasilyevAlexander VasilyevAlexander changed the title Memory leak in service_topo.cpp on Windows x64 with processor count more 32 Memory leak in service_topo.cpp on Windows x64 with core count more 32 Aug 16, 2018
@dr-pain
Copy link
Contributor

dr-pain commented Aug 17, 2018

Hello Alexander!

  1. now we are thinking about disabling ThreadPinning functionality at Windows. Do you use ThreadPinning?
    We fix it, if it will be decided to keep TheradPinning.
  2. glktsn has destructor (glktsn::FreeArrays() in the same source). all allocated tables free in it.
    Andrey

@VasilyevAlexander
Copy link
Author

  1. No, I do not use ThreadPinning. I discovered this effect wondering why the same program works on my laptop with 8 cores and 16 gb RAM, and run out of memory on 40 core workstation with 128gb RAM
  2. I saw this destructor. The problem is that it is never called since after the inconsistency in setChkProcessAffinityConsistency flag init is not set and next call to compute will call getL1CacheSize and getLLCacheSize. Both will call buildSystemTopologyTables and allocate additional memory but will not set init flag in glktsn and next call will allocate additional memory and so on. So destructor is never called. There should be calls to destructor after each check of an error before return without setting init flag but after allocation.

@dr-pain
Copy link
Contributor

dr-pain commented Aug 17, 2018

Hello!
Ok. Now I understand the usage case and see where problem is.
If you use open source DAAL, as quick workaround, I suggest to build libs with define DAAL_CPU_TOPO_DISABLED.
Andrey

@VasilyevAlexander
Copy link
Author

Hello, Andrey

Yes, there are two problems:

  1. Using macro _x86_64 instead of _M_X64 in service_topo.h (at least on with Windows with MSVC compiler)

  2. Incorrect clean up after error.

I have corrected issue #1 and everithing is working (at least for less than 64 cores)

Thanks,

Alexander

@dr-pain
Copy link
Contributor

dr-pain commented Aug 20, 2018

Hello Alexander!
We will fix p1 in upcoming DAAL version.
But I prefer to use _WIN64 macro because it already used in DAAL to determine Windows 64bits.
Andrey
PS: I have read about _M_X64/_M_AMD64/_WIN64 already :-)

@rlnx rlnx closed this as completed Mar 28, 2019
orrrrtem pushed a commit to orrrrtem/oneDAL that referenced this issue May 28, 2021
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