mmozeiko
Both.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 | fpl_internal XVisualInfo *GLXGetVisualInfo(const subplatform_x11::X11AppState &x11AppState, const X11VideoOpenGLState &glState, Display *display, int screen) { XVisualInfo *result = nullptr; const GLXApi &glxApi = glState.glxApi; const subplatform_x11::X11Api &x11Api = x11AppState.api; GLint attributes[] = { GLX_RENDER_TYPE, GLX_RGBA_BIT, GLX_DRAWABLE_TYPE, GLX_WINDOW_BIT, GLX_X_VISUAL_TYPE, GLX_TRUE_COLOR, GLX_RED_SIZE, 8, GLX_GREEN_SIZE, 8, GLX_BLUE_SIZE, 8, GLX_ALPHA_SIZE, 8, GLX_STENCIL_SIZE, 8, GLX_DEPTH_SIZE, 24, GLX_DOUBLEBUFFER, True, 0 }; // Find all frame buffer configs FPL_LOG("GLX", "Get FrameBuffer Configs for Display '%p' and Screen '%d':", display, screen); int fbAllConfigCount = 0; GLXFBConfig* fbAllConfigs = glxApi.glXChooseFBConfig(display, screen, attributes, &fbAllConfigCount); if(!fbAllConfigs || !fbAllConfigCount) { FPL_LOG("GLX", "Failed getting FrameBuffer Configs from Display '%p' and Screen '%d'!", display, screen); common::PushError("No FrameBuffer Configs returned"); return nullptr; } FPL_LOG("GLX", "Successfully got '%d' FrameBuffer Configs from Display '%p' and Screen '%d'", fbAllConfigCount, display, screen); for(int fbConfigIndex = 0; fbConfigIndex < fbAllConfigCount; ++fbConfigIndex) { const GLXFBConfig testFBConfig = fbAllConfigs[fbConfigIndex]; // Get visual info from frame buffer config XVisualInfo *visualInfo = (XVisualInfo*)glxApi.glXGetVisualFromFBConfig(display, testFBConfig); if(!visualInfo) { continue; } // Get alpha bits int testAlphaBits = GLXGetFrameBufferConfigAttrib(glxApi, display, testFBConfig, GLX_ALPHA_SIZE); // Found a suitable frame buffer config with visual if(testAlphaBits > 0) { int doubleBuffer = GLXGetFrameBufferConfigAttrib(glxApi, display, testFBConfig, GLX_DOUBLEBUFFER); int redBits = GLXGetFrameBufferConfigAttrib(glxApi, display, testFBConfig, GLX_RED_SIZE); int greenBits = GLXGetFrameBufferConfigAttrib(glxApi, display, testFBConfig, GLX_GREEN_SIZE); int blueBits = GLXGetFrameBufferConfigAttrib(glxApi, display, testFBConfig, GLX_BLUE_SIZE); int alphaBits = GLXGetFrameBufferConfigAttrib(glxApi, display, testFBConfig, GLX_ALPHA_SIZE); int depthBits = GLXGetFrameBufferConfigAttrib(glxApi, display, testFBConfig, GLX_DEPTH_SIZE); int colorDepth = redBits + greenBits + blueBits + alphaBits; result = visualInfo; FPL_LOG("GLX", "Successfully found a suitable Frame Buffer Config (Doublebuffer: %s, Color bits: %d, Depth bits: %d) with Visual '%p'", (doubleBuffer == True ? "Yes" : "No"), colorDepth, depthBits, visualInfo); break; } else { // Release unused visual info (Important!) x11Api.XFree(visualInfo); } } if (result == nullptr) { FPL_LOG("GLX", "Failed finding a suitable Frame Buffer Config for Display '%p'!", display); common::PushError("No FrameBuffer Configs returned"); } // Release all frame buffer configs (Important!) x11Api.XFree(fbAllConfigs); return(result); } |
mmozeiko
Sometimes it is false positive. Maybe library initializes some global singleton that allocates memory just once, first time. And after that keeps it alive while process is running.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 | static GlobalCache* cache; GLXFBConfig* glXChooseFBConfig(...) { if (cache == NULL) { cache = malloc(...); SetupCache(cache) } // .. do actual choosing GLXFBConfig* result = malloc(...); // TODO: fill it up return result; } void XFree(void* ptr) { free(ptr); } |
mmozeiko
This can happen because of something like this:
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 static GlobalCache* cache; GLXFBConfig* glXChooseFBConfig(...) { if (cache == NULL) { cache = malloc(...); SetupCache(cache) } // .. do actual choosing GLXFBConfig* result = malloc(...); // TODO: fill it up return result; } void XFree(void* ptr) { free(ptr); }
As you can see the returned pointer is freed, but global cache is not. Because it is allocated only once. Its a "singleton pattern". But valgrind will report this as a leak. This is a very common false positive in system libraries - both on Linux, and Windows.
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 | fpl_platform_api void *MemoryAllocate(const size_t size) { // @NOTE(final): MAP_ANONYMOUS ensures that the memory is cleared to zero. // Allocate empty memory to hold the size + some arbitary padding + the actual data size_t newSize = sizeof(size_t) + sizeof(uintptr_t) + size; void *basePtr = ::mmap(nullptr, newSize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); // Write the size at the beginning *(size_t *)basePtr = newSize; // The resulting address starts after the arbitary padding void *result = (uint8_t *)basePtr + sizeof(size_t) + sizeof(uintptr_t); return(result); } fpl_platform_api void MemoryFree(void *ptr) { // Free the base pointer which is stored to the left at the start of the size_t void *basePtr = (void *)((uint8_t *)ptr - (sizeof(uintptr_t) + sizeof(size_t))); size_t storedSize = *(size_t *)basePtr; ::munmap(basePtr, storedSize); } |
Finalspace
FPL does not use a singleton pattern,...
1 | void *basePtr = ::mmap(nullptr, newSize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); |
mmozeikoFinalspace
FPL does not use a singleton pattern,...
You misunderstood me. I'm not talking about FPL code here. I'm talking about X11 implementation (libX11.so or whatever). If they use singleton, then that will show as false positive in valgrind.
1 void *basePtr = ::mmap(nullptr, newSize, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
As an unrelated topic - valgrind and similar tools won't show up this as memory leak in case you forgot to munmap it. They check the heap allocations for leaks, not mmap/sbrk/whatever syscalls.
Btw, I've looked more into your memory allocation functions and there are few issues/improvements:
1) first of all, your MemoryStackAllocate function is wrong. You cannot return memory allocated on stack from function. When function returns, memory is freed. It's pointless to return it. The best you can do is make a macro if you really want it.
2) MemoryAlignedAllocate is kind of pointless if it uses system allocator. Because system allocators return page aligned allocations. So they all will be at least 4k aligned.
mmozeiko
Wait, I'm not talking about aligned_malloc. I'm talking about your functions which don't use aligned_malloc.
Maybe I don't understand the purpose of your MemoryAllocate/MemoryAlignedAllocate functions? From code it looks like they are supposed to be wrappers around system allocators (VirtualAlloc/mmap), not heap alloctor (HeapAlloc/malloc). Yes, heap allocators align only on max 8. That's why thereis aligned_malloc or posix_memalign. But system allocators on modern OS'es always align at least on 4k. I do not know situation where you would want more than 4k alignment. Unless you are writing kernel and dealing with DMA or smth.... but that would require a lot more changes anyway. So imho your AlignedAllocate function is pointless.
Also you are using MemoryAlignedAllocate in your FPL to allocate bunch of very tiny allocations - event queue, state, etc.. This is waste of memory (and cache space). I suggest to replace them with heap allocations.
Finalspace
Regarding the purpose of MemoryAllocate/MemoryAlignedAllocate: Its meant to be a call directly to allocate a block of memory from the OS directly ...
mmozeikoFinalspace
Regarding the purpose of MemoryAllocate/MemoryAlignedAllocate: Its meant to be a call directly to allocate a block of memory from the OS directly ...
This is exactly my point. OS directly gives you 4k aligned memory (via VirtualAlloc or mmap). Why do you want to align it again in MemoryAlignedAllocate?
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 | class procedure TPasMPMemory.AllocateAlignedMemory(var p;Size:TPasMPInt32;Align:TPasMPInt32=PasMPCPUCacheLineSize); var Original,Aligned:pointer; Mask:ptruint; begin if (Align and (Align-1))<>0 then begin Align:=TPasMPMath.RoundUpToPowerOfTwo(Align); end; Mask:=Align-1; inc(Size,((Align shl 1)+SizeOf(pointer))); GetMem(Original,Size); FillChar(Original^,Size,#0); Aligned:=pointer(ptruint(ptruint(Original)+SizeOf(pointer))); if (Align>1) and ((ptruint(Aligned) and Mask)<>0) then begin inc(ptruint(Aligned),ptruint(ptruint(Align)-(ptruint(Aligned) and Mask))); end; pointer(pointer(ptruint(ptruint(Aligned)-SizeOf(pointer)))^):=Original; pointer(pointer(@p)^):=Aligned; end; class procedure TPasMPMemory.FreeAlignedMemory(const p); var pp:pointer; begin pp:=pointer(pointer(@p)^); if assigned(pp) then begin pp:=pointer(pointer(ptruint(ptruint(pp)-SizeOf(pointer)))^); FreeMem(pp); end; end; |
Now i hope that this is clear now.I don't think you understand what I'm trying to say :) I completely understand how aligned_alloc works or why it is necessary, or how to implement it. I have implemented such function many times myself. And I'm not saying "aligned_alloc" does not make sense. I am only saying that your fpl::MemoryAlignedAllocate does not make sense, because it does uses fpl::MemoryAllocate which is defined as system allocator (which guarantees alignment, always!) and it does not implement custom heap, like C malloc or pascal GetMem does.
1 2 3 4 | void *MemoryAlignedAllocate(const size_t size, const size_t alignment) { FPL_ASSERT(alignment <= 4096); return VirtualAlloc(NULL, size, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE); } |
mmozeiko
Of course "aligned" alloc function makes sense if it internally uses function which does not guarantee aligment.
In C aligned_alloc exists because it uses malloc. And malloc does not guarantee big enough alignment.
Or from your example in pascal - AllocateAlignedMemory makes sens because GetMem does not guarantee big enough alignment.
Your MemoryAlignedAllocate uses MemoryAllocate which internally guarantees 4k alignment. You are aligning already aligned memory (with few hacks around storing size in "header", that should be improved on API level). It does not make sense.
Now i hope that this is clear now.I don't think you understand what I'm trying to say :) I completely understand how aligned_alloc works or why it is necessary, or how to implement it. I have implemented such function many times myself. And I'm not saying "aligned_alloc" does not make sense. I am only saying that your fpl::MemoryAlignedAllocate does not make sense, because it does uses fpl::MemoryAllocate which is defined as system allocator (which guarantees alignment, always!) and it does not implement custom heap, like C malloc or pascal GetMem does.
Here's how I would implement your fpl::MemoryAlignedAllocate function on Windows:
1 2 3 4 void *MemoryAlignedAllocate(const size_t size, const size_t alignment) { FPL_ASSERT(alignment <= 4096); return VirtualAlloc(NULL, size, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE); }
As you can see, it does not need to use alignment, unless somebody would ask for >4k aligned memory. If you really really would want to support, then sure handle it such special case. But really, why would anybody ask for more-than-page-size aligned memory...