About tracerealloc()

[Plombo]

Argh, tracerealloc() is a mess again!  I worked for about a month to phase it out of the preprocessor so that I could remove it from the source code once and for all.  But today, I found that it is once again in use in another part of the engine!

If we are going to keep the function, some things need to be changed.  The way I had it before worked reasonably well, and now it's all kinds of broken.  This is not the fault of a single individual, and I suppose it's partly mine for putting the function there in the first place.

To anallyst:
I put the "oldlen" parameter there for a reason.  It is necessary on Windows since the Windows version uses a fallback to replace the realloc() implementation in the Windows C runtime, as I mentioned in my comments in the source code.  Realloc() on Windows frequently returns NULL pointers even when there is plenty of memory left, so the fallback is needed for reliable behavior on that platform.  The "oldlen" parameter is used to know how many bytes to memcpy in the alternative implementation.

Sorry if this sounds like I'm mad at you; I'm really not.

To SX:
Commenting the Windows fallback was the wrong fix to make.  Now tracerealloc() on Windows will frequently return NULL pointers for allocations of a few kilobytes even when there are several gigabytes of memory left.

To both of you:
Now that I've gotten my rant out, please do not do anything yet.  I will clean up this mess myself.

[anallyst]

To anallyst:
I put the "oldlen" parameter there for a reason.  It is necessary on Windows since the Windows version uses a fallback to replace the realloc() implementation in the Windows C runtime, as I mentioned in my comments in the source code.  Realloc() on Windows frequently returns NULL pointers even when there is plenty of memory left, so the fallback is needed for reliable behavior on that platform.

i figured as much myself (after SX's change), though it would require some algorithm change. are you sure that windows realloc don't work? i can't believe that really

basically i find that lib rather disturbing and hard to understand... it's stilly buggy in 64 bits, i disabled it for my test builds, since i find gdb + valgrind a more suiting solution...

i will gladly leave it untouched for the time being 

[anallyst]

for the oldsize to work smoothly i'd recommend that tracemalloc saves the size to the location retrieved and use the info from there, so it will relieve us from bookkeeping in the game code

[Plombo]

Oops, you replied before I finished my edit.

i figured as much myself (after SX's change), though it would require some algorithm change. are you sure that windows realloc don't work? i can't believe that really

Yup.  I'm sure because it caused a fiasco a few months ago, when I was using the function in my preprocessor.  It's a really lousy implementation on Microsoft's part.


And you replied again before I could finish this post.

for the oldsize to work smoothly i'd recommend that tracemalloc saves the size to the location retrieved and use the info from there, so it will relieve us from bookkeeping in the game code

I've considered that, but it would increase memory usage significantly for mods using scripts.  Depending on the volume of scripts used in a mod, the script engine makes hundreds to thousands of tiny allocations, especially of the ScriptVariant structure but also others.  Tacking 4 bytes onto each allocation would thus create a noticeable increase in memory usage on platforms where memory is a very scarce resource.

[anallyst]

I've considered that, but it would increase memory usage significantly for mods using scripts.  Depending on the volume of scripts used in a mod, the script engine makes hundreds to thousands of tiny allocations, especially of the ScriptVariant structure but also others.  Tacking 4 bytes onto each allocation would thus create a noticeable increase in memory usage on platforms where memory is a very scarce resource.

hmm its only enabled in debug mode so that should be no problem
for 1000 mallocs it's an additional 4KB, so thats not so huge anyway

[Plombo]

I've considered that, but it would increase memory usage significantly for mods using scripts.  Depending on the volume of scripts used in a mod, the script engine makes hundreds to thousands of tiny allocations, especially of the ScriptVariant structure but also others.  Tacking 4 bytes onto each allocation would thus create a noticeable increase in memory usage on platforms where memory is a very scarce resource.

hmm its only enabled in debug mode so that should be no problem
for 1000 mallocs it's an additional 4KB, so thats not so huge anyway

The problem is that the engine uses tracerealloc() even when it's not in debug mode.

I'm currently debating with myself whether to improve the existing tracerealloc() function, making it easier to get this type of functionality in the engine, or remove it since it creates an additional maintenance burden on tracelib.

[SX]

Wait a second....
Its impossible that the current implementation of fakerealloc() ever worked.  Building natively on windows this would have been impossible.  I get an error of undefined variable for oldlen.  Meaning that tracelib has not been in use for a while or it has not been tested on Windows.

Now its been a while since I've worked on the engine, but if I'm not mistaken the tracelib was disabled in RELEASE mode and enabled in DEBUG mode by myself long long ago. 

The reason for this is all the memory fragmentation it creates since most of the allocations in the system are in small chunks.  Less then 1K Bytes and should be only used for checking for memory leaks.  That is why I left this library in and only available in DEBUG mode.

To SX:
Commenting the Windows fallback was the wrong fix to make.  Now tracerealloc() on Windows will frequently return NULL pointers for allocations of a few kilobytes even when there are several gigabytes of memory left.

Furthermore, as I stated it was temporary commented out so I could get some feedback from whom ever implemented this function.  Otherwise compilation would fail.

[SX]

Lets all meet in the chat room and discuss this.

[anallyst]

The problem is that the engine uses tracerealloc() even when it's not in debug mode.

well, in debug mode it just calls realloc, and doesnt save anything. it costs only a handful of cpu cycles for calling a second function

I'm currently debating with myself whether to improve the existing tracerealloc() function, making it easier to get this type of functionality in the engine, or remove it since it creates an additional maintenance burden on tracelib.

i have read the thread you linked, i don't believe that windows realloc is broken because a single users thinks so.
in the opposite, a whole bunch of applications wouldn't work in windows. a google search for "windows realloc broken" also doesn't bring up anything solid. so basically, you could just remove the workaround function and fix the 64bit issues, where it accesses illegal memory regions, as shown by valgrind.

[Plombo]

i have read the thread you linked, i don't believe that windows realloc is broken because a single users thinks so.
in the opposite, a whole bunch of applications wouldn't work in windows. a google search for "windows realloc broken" also doesn't bring up anything solid. so basically, you could just remove the workaround function and fix the 64bit issues, where it accesses illegal memory regions, as shown by valgrind.

It wasn't a single user "thinking so".  It was the OpenBOR preprocessor not working for a single user until I replaced tracerealloc() with fakerealloc() on Windows, at which point it began working flawlessly.

But like I said in the chat...if you can make it work (which shouldn't be too hard) tracerealloc() can work again.

Wait a second....
Its impossible that the current implementation of fakerealloc() ever worked.  Building natively on windows this would have been impossible.  I get an error of undefined variable for oldlen.  Meaning that tracelib has not been in use for a while or it has not been tested on Windows.

Now its been a while since I've worked on the engine, but if I'm not mistaken the tracelib was disabled in RELEASE mode and enabled in DEBUG mode by myself long long ago.  

The reason for this is all the memory fragmentation it creates since most of the allocations in the system are in small chunks.  Less then 1K Bytes and should be only used for checking for memory leaks.  That is why I left this library in and only available in DEBUG mode.

It was almost exactly a year ago, last February IIRC...not sure if that was "long long ago".  Tracelib has been in use and tested on Windows, see my comment below.  It only broke today.

Furthermore, as I stated it was temporary commented out so I could get some feedback from whom ever implemented this function.  Otherwise compilation would fail.

I implemented the function, but it stopped working when anallyst accidentally changed the signature without changing the function or the macro redefining tracerealloc() to use it.  The function worked flawlessly before then.

[DavidC99]

It wasn't a single user "thinking so".  It was the OpenBOR preprocessor not working for a single user until I replaced tracerealloc() with fakerealloc() on Windows, at which point it began working flawlessly.

Oh, that actually made a difference?  I've been under the impression this whole time that had absolutely nothing to do with issues at that time.  Go figure...   

I don't think it's necessarily fair to say that realloc is broken on Windows, but I do remember hearing of a few potential quirks regarding implementations.  Either way, I tend to avoid using it anyway no matter what platform I'm using.

So... now that I've contributed nothing to this discussion, carry on.   

[Plombo]

It wasn't a single user "thinking so".  It was the OpenBOR preprocessor not working for a single user until I replaced tracerealloc() with fakerealloc() on Windows, at which point it began working flawlessly.

Oh, that actually made a difference?  I've been under the impression this whole time that had absolutely nothing to do with issues at that time.  Go figure...   

Yeah...realloc() stopped causing problems on Windows when it stopped being used.

There was more than one issue with the preprocessor at that time, though, and it wasn't clear at the time that they were all distinct since all of them resulted in crashing.

I don't think it's necessarily fair to say that realloc is broken on Windows, but I do remember hearing of a few potential quirks regarding implementations.  Either way, I tend to avoid using it anyway no matter what platform I'm using.

And what I'm somewhat afraid of is that other platforms besides Windows could have their own realloc() quirks.  It seems like a rather difficult thing to implement well - if Microsoft can't get it entirely right, I have to wonder how many platforms actually do.


The conclusion I've reached after thinking about this for the past few days is that the best compromise between functionality and portability/reliability would be to add the "oldlen" parameter back to tracerealloc.  Then we could use a custom wrapper (on every platform) that first tries to use realloc(), and falls back on tracemalloc+memcpy+tracefree if realloc fails.

I don't think it's too much to ask that the caller keep track of the size of the buffer.  The way I see it, the main reason to call realloc() is when a buffer is too small - and you can't know if it's too small if you don't know its size already.

[anallyst]

it is a PITA to keep track of that in i.e. recursive functions, you'd have to write all your algorithm's to take care of it, or insert yet another list/global variable to track it.
seriously, realloc ISN'T buggy on windows, because if it was, windows itself would crash constantly.
or do you believe that MS itself doesnt use realloc, and implements similar algorithms by foot in every needed context, as we did ? LOL.

as you can see, the build works fine for everybody, most ppl are telling how cool the new 70+ builds are.

oh, and realloc is everything else than complicated too implement. it's just a lookup in the internal structure, eventually resize the allocated block, if there's available space in the end; or allocate a new block and do a memcpy.
do you really believe that microsoft's engineer's are too stupid to implement that 10 liner correctly ?

come on, if you still don't believe me google for "windows realloc fail" or so... you'll just gonna find forum entries from ppl who failed to use it correctly.

[Plombo]

it is a PITA to keep track of that in i.e. recursive functions, you'd have to write all your algorithm's to take care of it, or insert yet another list/global variable to track it.

If we ever have a need for realloc in a recursive function, we can handle that problem when it comes up.

seriously, realloc ISN'T buggy on windows, because if it was, windows itself would crash constantly.
or do you believe that MS itself doesnt use realloc, and implements similar algorithms by foot in every needed context, as we did ? LOL.

Well, I wouldn't say that MS uses the C function realloc() all that often.  The Windows API has its own functions for memory allocation (HeapRealloc in this case) that Microsoft prefers for developers to use instead of standard C library functions.  I'd assume they use them themselves, instead of just telling everyone else to use them.

This isn't the actual problem though, see below.

as you can see, the build works fine for everybody, most ppl are telling how cool the new 70+ builds are.

I have seen one report about how well 307X works.  And it says that the script compiler is messed up.  Where are all these people telling how cool they are?

oh, and realloc is everything else than complicated too implement. it's just a lookup in the internal structure, eventually resize the allocated block, if there's available space in the end; or allocate a new block and do a memcpy.
do you really believe that microsoft's engineer's are too stupid to implement that 10 liner correctly ?

The problem isn't that realloc() is broken, it's that malloc() and realloc() in Windows return NULL pointers when the Windows Heap object is extremely fragmented.  The Windows heap manager is unable to prevent fragmentation with certain allocation patterns, specifically the pattern of making tons of very small allocations and freeing them afterwards.  Sound familiar?  It's what the OpenBOR script engine does, and it affects the entire program.

Some more details about Windows Heaps and fragmentation can be found here.

So the Windows engineers aren't stupid, but OpenBOR's allocation pattern is a corner case that Windows does not handle well.  That's why you don't get tons of Google results for "Windows realloc broken" or anything of that nature.

And just in case you still believe that Windows allocation is perfect and never returns NULL when there is memory left, please note this sentence from Microsoft's own malloc() documentation:

Always check the return from malloc, even if the amount of memory requested is small.

That sentence is there for a reason.

[anallyst]

SURE you shall check the return value ! EVERY function that allocates ram can return NULL if no block of the requested size is available
and NO, the specific use cases of OpenBOR DON'T LEAD to errors

pls stop discussing about that bullshit, or i have to invite the trolls from the ##C channel or reddit to teach you

Also, I've thought of a factor that might actually be important.  Does it only fail when the total size of a script, with all preprocessing finished, is more than 4 KB?  If so, it could be a realloc() problem.

Okay, my current theory is that Windows + realloc() == fail.  Do you know if Windows has any issues with realloc() usage?

Um.................right.

come on, if you take this vague feeling you had at the time serious, there's something wrong

and YES, even Microsoft uses realloc, because it simplifies memory management a lot, and leads to compacter and cleaner code