TDM Engine Development Page

[nbohr1more]

Hmm. Your integrating this into Heap.cpp?

 

[revelator]

Aye since its the only place malloc is really used besides 3rd party code, its also engine global so the garbadge collector has access to the entire source game dll's included

[revelator]

Boehm GC also has a leak detector and i used that to hunt down the leak,

funny enough it was the malloc in Mem_Alloc16 after i changed it to use _aligned malloc there seems to be no leaks left

Warning though enabling the leakdetector will slow the engine somewhat so only use it for debugging, GC will still print problems if they occur, these will be output to a text file with enginename.gc.log

and only if problems occur so its safe to let it run in non debug mode.

[nbohr1more]

It seems that projects keep re-discovering Mem_Alloc16 has a problem in github.

 

Sadly, I remember awhile back doing some generic searches on Doom 3 memory or performance and found a branch

that had some unique changes to Heap.cpp. I (stupidly) never bookmarked it thinking my search terms would find it again

easily. Now I've spent two days of and on and cannot find that branch. I wish you could get a tree view of changes to just one

file at github because I'm almost certain that this area was rarely touched

[revelator]

/*
==================
Mem_Alloc16
==================
*/
void *Mem_Alloc16(const int size) {
if (!size) {
 return NULL;
}
if (!mem_heap) {
 const int paddedSize = (size + 15) & ~15;
#ifdef CRASH_ON_STATIC_ALLOCATION
 *((int *)0x0) = 1;
#endif
 return _aligned_malloc(paddedSize, 16);
}
void *mem = mem_heap->Allocate16(size);
// make sure the memory is 16 byte aligned
assert((((int)mem) & 15) == 0);
return mem;
}
/*
==================
Mem_Free16
==================
*/
void Mem_Free16(void *ptr) {
if (!ptr) {
 return;
}
if (!mem_heap) {
#ifdef CRASH_ON_STATIC_ALLOCATION
 *((int *)0x0) = 1;
#endif
 _aligned_free(ptr);
 return;
}
// make sure the memory is 16 byte aligned
assert((((int)ptr) & 15) == 0);
mem_heap->Free16(ptr);
}

 

Cant say what changes there might have been in the other engine but heres the fix for Mem_Alloc16 and Mem_Free16

[revelator]

Comitted the GC wrapper interface to Heap.cpp in idlib changes are comitted on github as well.

[SteveL]

What's the benefit of the Boehm wrapper? Not challenging it, I just don't know :-)

 

Come to think of it, taaaki was trying to track down some memory leaks earlier in this dev cycle but I don't know whether he got to the bottom of it.

 

I would guess our big memory allocations come from r_framealloc which is how the front end passes all its data to the back end. The back end doesn't allocate memory. All drawSurfs, all viewEntities, and viewLights along with their skinned vertex data are allocated on the heap every frame so the backend can read it while the front end modifies it for the next frame. We don't have the two processes running simultaneously, but that architecture is still in place. That memory is supposed to be freed every frame automatically when the back end is done with it. I guess that much must be (mostly) working or we'd notice a big leak right away.

[OrbWeaver]

Maybe Orbweaver would be a good candidate to review this patch as he did VBO work in DR ???

 

I'm not really an expert on such matters; I know enough about how to use VBOs to use them in DarkRadiant in a fairly basic way, but without any in-depth knowledge of the rendering pipeline inside the game itself I probably wouldn't be able to provide much useful input.

[revelator]

Its an automatic garbadge collector for malloc and friends, mostly used in gcc's java backend.

The benefit is that it tracks and if possible applies correction to malloc errors if an error occurs you get a small logfile with information about something being amiss,

so it also eases hunting down those kind of problems. i did correct the leak in idlib thanks to this little tool was actually a small mistake where plain malloc was used in a function that needs 16 byte

aligned malloc and same for free. The wrappers i wrote use GC as malloc if defined and system malloc if not,

in case it puts to much overhead on the engine you can just undefine it but so far it seems to run fine with GC enabled all the time.

[revelator]

Ah yes r_framealloc had a hunch that might be the one pushing those huge allocations, was actually not the huge allocations themself that caused the leak see above.

Only other engine i seen allocate this much memory was the old tenebrae quake engine which allocated 1 GB at start ouch, but then again it was the first quake1 engine with realtime lighting and bumpmaps

so it makes sense.

[Tels]

The Linux build is broken:

 

 

 

nizip -Iinclude/libjpeg -Iinclude/devil -I. renderer/Image_load.cpp
renderer/Image_load.cpp: In member function ‘void idImage::GenerateImage(const byte*, int, int, textureFilter_t, bool, textureRepeat_t, textureDepth_t)’:
renderer/Image_load.cpp:600: warning: suggest parentheses around ‘&&’ within ‘||’
renderer/Image_load.cpp: In member function ‘bool idImage::CheckPrecompressedImage(bool)’:
renderer/Image_load.cpp:1364: warning: comparison between signed and unsigned integer expressions
renderer/Image_load.cpp: In member function ‘void idImage::PurgeImage()’:
renderer/Image_load.cpp:1659: warning: comparison between signed and unsigned integer expressions
renderer/Image_load.cpp: In member function ‘void idImage::Bind()’:
renderer/Image_load.cpp:1705: warning: comparison between signed and unsigned integer expressions
renderer/Image_load.cpp:1750: warning: comparison between signed and unsigned integer expressions
renderer/Image_load.cpp:1755: warning: comparison between signed and unsigned integer expressions
renderer/Image_load.cpp:1760: warning: comparison between signed and unsigned integer expressions
renderer/Image_load.cpp: In member function ‘void idImage::BindFragment()’:
renderer/Image_load.cpp:1802: warning: comparison between signed and unsigned integer expressions
renderer/Image_load.cpp: In member function ‘void idImage::CopyDepthbuffer(int, int, int, int, bool)’:
renderer/Image_load.cpp:1927: error: ‘use_qglReadBuffer’ was not declared in this scope
renderer/Image_load.cpp:1937: error: ‘use_qglTexImage2D’ was not declared in this scope
renderer/Image_load.cpp:1938: error: ‘use_qglCopyTexSubImage2D’ was not declared in this scope
renderer/Image_load.cpp:1943: error: ‘use_qglCopyTexSubImage2D’ was not declared in this scope
renderer/Image_load.cpp:1945: error: ‘use_qglTexParameterf’ was not declared in this scope
renderer/Image_load.cpp: In member function ‘int idImage::StorageSize() const’:
renderer/Image_load.cpp:2043: warning: comparison between signed and unsigned integer expressions
renderer/Image_load.cpp: At global scope:
renderer/Image_load.cpp:23: warning: ‘versioned’ defined but not used
include/boost/system/error_code.hpp:214: warning: ‘boost::system::posix_category’ defined but not used
include/boost/system/error_code.hpp:215: warning: ‘boost::system::errno_ecat’ defined but not used
include/boost/system/error_code.hpp:216: warning: ‘boost::system::native_ecat’ defined but not used
scons: *** [build/release/core/renderer/Image_load.o] Error 1
renderer/Image_init.cpp: At global scope:
renderer/Image_init.cpp:228: warning: ‘void R_AlphaRampImage(idImage*)’ defined but not used
renderer/Image_init.cpp:381: warning: ‘void R_RGB8Image(idImage*)’ defined but not used
renderer/Image_init.cpp:449: warning: ‘void CreateSquareLight()’ defined but not used
renderer/Image_init.cpp:495: warning: ‘void CreateFlashOff()’ defined but not used
scons: building terminated because of errors.

 

 

Edited November 17, 2014 by Tels

[SteveL]

The Linux build is broken:

Is that from our svn trunk or from revelator's branch?

[Tels]

Is that from our svn trunk or from revelator's branch?

 

From the main trunk, I haven't switched branches. Revision 6143.

 

(Oh, and one code review comment: Please use more const in parameters. The new function idImage::CopyDepthbuffer modifies some of its arguments (imageHeight & width, or at least it looks like it does?) but keeps others (x,y) constant. That looks odd and makes it harder to see whats going on

Edited November 17, 2014 by Tels

[SteveL]

Ok. The warnings are coming from old code but the errors are in the fixed CopyDepthBuffer, although I don't remember that we added any new calls to it. I'll try to work out what might be upsetting it later, though I don't have a Linux build to test on.

[SteveL]

And again sorry for clarity, is this the 2.03 trunk, or the 2.02 trunk with my particle patch applied? Probably both though unless I screwed up the patch.

[Tels]

And again sorry for clarity, is this the 2.03 trunk, or the 2.02 trunk with my particle patch applied? Probably both though unless I screwed up the patch.

 

Sorry, should have been more verbose:

 

$ update
Path: .
URL: https://svn.thedarkmod.com/svn/darkmod_src/trunk
Repository Root: https://svn.thedarkmod.com/svn/darkmod_src
Repository UUID: 49c82d7f-2e2a-0410-a16f-ae8f201b507f
Revision: 6144
Node Kind: directory
Schedule: normal
Last Changed Author: stevel
Last Changed Rev: 6143
Last Changed Date: 2014-11-15 19:36:41 +0100 (Sat, 15 Nov 2014)

 

AFAICS the errors come from re-defined names:

 

$ ack use_qglReadBuffer

sys/linux/qgl_enforce.h

1361:#define glReadBuffer use_qglReadBuffer

 

Maybe I'm missing a .h file or a dev package is not installed?

[Tels]

Actually, the file sys/linux/qgl_enforce.h looks bogus to me. It contains a few hundred empty lines, than hundreds of redefines, but I can't find anywhere what they refer to, not even google.

 

Even if we keep it, the 1400 or so empty lines should be removed from the top.

[revelator]

qgl_enforce.h is not used not even on linux its part of the broken gllog and was disabled by id (remnant of idtech 3), the qgl errors seems to be from some missing exports in the linux build hmm.

Ill have a look at the ones you posted though im not on linux i might be able to fix them.

[revelator]

Hmm do these errors pop up anywhere else in the build ? the use_qglsomething only exist in qgl_enforce.h and since its not even included anywhere i have a hard time figuring out why the compiler complains about these functions there not only used in image_load.cpp.

 

If you could do me a favour try compiling the one from my github, you will probably have to get the glew package for your linux distro, im curious to see if it builds ok on linux.

[SteveL]

So how is qgl_enforce.h getting included?

That #define explains the error messages. I knew there were no explicit calls to any use_qgl* functions in that function, but it's using macros to swap out the functon names.

Will it compile if you rename qgl_enforce.h? Probably not but then you should get an include error which'll tell you where it's being included erroneously.

 

(Oh, and one code review comment: Please use more const in parameters. The new function idImage::CopyDepthbuffer modifies some of its arguments (imageHeight & width, or at least it looks like it does?) but keeps others (x,y) constant. That looks odd and makes it harder to see whats going on

NB that's a function declaration from the original engine code. We added one param. I agree completely that more-const is better than less-const, but it's a question of good style rather than code correctness in this spot as they're built-ins passed by value, so there are no possible side-effects. In this spot I'd prioritise the rules that say (1) you align new code to the surrounding code, hence the one new param matches the existing ones, and (2) you don't change what ain't broken when you already have a huge testing to-do list!

That said, it *is* confusing that two of the parameters get modified by another function that looks like it won't modify them. That one's my pet hate: seeing values passed to a function to be modified by non-const reference instead of a non-const pointer, which means there's no clue whatsoever for the reader that the values will get modified. In our defence, that's existing code.

 

ps I'm not using the royal 'we' here "We" and "our" means revelator and I, we collaborated on getting that (exsiting) function working.

[SteveL]

At the end of qgl.h:

#if defined( __linux__ )

//GLX Functions
extern XVisualInfo * (*qglXChooseVisual)( Display *dpy, int screen, int *attribList );
extern GLXContext (*qglXCreateContext)( Display *dpy, XVisualInfo *vis, GLXContext shareList, Bool direct );
extern void (*qglXDestroyContext)( Display *dpy, GLXContext ctx );
extern Bool (*qglXMakeCurrent)( Display *dpy, GLXDrawable drawable, GLXContext ctx);
extern void (*qglXSwapBuffers)( Display *dpy, GLXDrawable drawable );
extern GLExtension_t (*qglXGetProcAddressARB)( const GLubyte *procname );

// make sure the code is correctly using qgl everywhere
// don't enable that when building glimp itself obviously..
#if !defined( GLIMP )
   #include "../sys/linux/qgl_enforce.h"
#endif

#endif // __linux__

[Tels]

Hmm do these errors pop up anywhere else in the build ? the use_qglsomething only exist in qgl_enforce.h and since its not even included anywhere i have a hard time figuring out why the compiler complains about these functions there not only used in image_load.cpp.

 

If you could do me a favour try compiling the one from my github, you will probably have to get the glew package for your linux distro, im curious to see if it builds ok on linux.

 

Could you please remind me how to checkout from git? I guess it stil has a linuxBuild.sh I can then run?

 

If the file is not used at all, we should delete it promptly, it only wastes human resources figuring out

 

Actually: renderer/qgl.h line 537 or so:

 

// make sure the code is correctly using qgl everywhere
// don't enable that when building glimp itself obviously..
#if !defined( GLIMP )
   #include "../sys/linux/qgl_enforce.h"
#endif

Edited November 17, 2014 by Tels

[Tels]

So how is qgl_enforce.h getting included?

That #define explains the error messages. I knew there were no explicit calls to any use_qgl* functions in that function, but it's using macros to swap out the functon names.

Will it compile if you rename qgl_enforce.h? Probably not but then you should get an include error which'll tell you where it's being included erroneously.

 

 

NB that's a function declaration from the original engine code. We added one param. I agree completely that more-const is better than less-const, but it's a question of good style rather than code correctness in this spot as they're built-ins passed by value, so there are no possible side-effects. In this spot I'd prioritise the rules that say (1) you align new code to the surrounding code, hence the one new param matches the existing ones, and (2) you don't change what ain't broken when you already have a huge testing to-do list!

That said, it *is* confusing that two of the parameters get modified by another function that looks like it won't modify them. That one's my pet hate: seeing values passed to a function to be modified by non-const reference instead of a non-const pointer, which means there's no clue whatsoever for the reader that the values will get modified. In our defence, that's existing code.

 

Yeah, I agree with the "dont fix whats broken"; but OTOH, the code never gets better if we only add in the old style. To our defense, we do have at least 3 different styles and the mess isn't getting much better. (Large cleanup efforts are not really a priority given the resources).

 

Still I'd wish we could stop spending so much time looking at the code just because it has these traps

 

ps I'm not using the royal 'we' here "We" and "our" means revelator and I, we collaborated on getting that (exsiting) function working.

 

I think we all sit in the same boat

[revelator]

git clone github address

 

and the m4 defines for using qgl_enforce.h is disabled in scons hence why i was a bit curious as to why it still gets included but yeah did not notice that include in qgl.h it seems to not exist in my unmodified vanilla checkout curious still its bad behaviour as pointed out and the best thing to do would be to kill it untill its dead i also suspect that it would interfer with the new version based on glew since the engine now uses the real gl names instead of the pointers so more work to be done i suspect.

[revelator]

small function you can implement at once here, fixes videoram detection.

 

/*
================
Sys_GetVideoRam
returns in megabytes
This function works but returned negative sizes.
Fixed now.
================
*/
int Sys_GetVideoRam(void) {
#ifdef ID_DEDICATED
return 0;
#else
int retSize = 64;
CComPtr<IWbemLocator> spLoc = NULL;
HRESULT hr = CoCreateInstance(CLSID_WbemLocator, 0, CLSCTX_SERVER, IID_IWbemLocator, (LPVOID *)&spLoc);
if (hr != S_OK || spLoc == NULL) {
 if (retSize < 0) {
  return retSize = -retSize;
 }
 else {
  return abs(retSize);
 }
}
CComBSTR bstrNamespace(_T("\\\\.\\root\\CIMV2"));
CComPtr<IWbemServices> spServices;
// Connect to CIM
hr = spLoc->ConnectServer(bstrNamespace, NULL, NULL, 0, NULL, 0, 0, &spServices);
if (hr != WBEM_S_NO_ERROR) {
 if (retSize < 0) {
  return retSize = -retSize;
 }
 else {
  return abs(retSize);
 }
}
// Switch the security level to IMPERSONATE so that provider will grant access to system-level objects.
hr = CoSetProxyBlanket(spServices, RPC_C_AUTHN_WINNT, RPC_C_AUTHZ_NONE, NULL, RPC_C_AUTHN_LEVEL_CALL, RPC_C_IMP_LEVEL_IMPERSONATE, NULL, EOAC_NONE);
if (hr != S_OK) {
 if (retSize < 0) {
  return retSize = -retSize;
 }
 else {
  return abs(retSize);
 }
}
// Get the vid controller
CComPtr<IEnumWbemClassObject> spEnumInst = NULL;
hr = spServices->CreateInstanceEnum(CComBSTR("Win32_VideoController"), WBEM_FLAG_SHALLOW, NULL, &spEnumInst);
if (hr != WBEM_S_NO_ERROR || spEnumInst == NULL) {
 if (retSize < 0) {
  return retSize = -retSize;
 }
 else {
  return abs(retSize);
 }
}
ULONG uNumOfInstances = 0;
CComPtr<IWbemClassObject> spInstance = NULL;
hr = spEnumInst->Next(10000, 1, &spInstance, &uNumOfInstances);
if (hr == S_OK && spInstance) {
 // Get properties from the object
 CComVariant varSize;
 hr = spInstance->Get(CComBSTR(_T("AdapterRAM")), 0, &varSize, 0, 0);
 if (hr == S_OK) {
  retSize = varSize.intVal / (1024 * 1024);
  if (retSize == 0) {
   retSize = 64;
  }
 }
}
if (retSize < 0) {
 return retSize = -retSize;
}
return abs(retSize);
#endif
}

 

This makes auto setting system spec viable again before this fix it still worked but returned negative numbers so if your card had say 2 GB memory it would return -2048 instead of 2048 causing the detection to fail.