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 Manager work + general clean-up #732

Open
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@madebr
Contributor

madebr commented Oct 8, 2015

  • My linker did not add some functions to the jagamex86_64.so. so explicitly add them by adding a static-specifier to the function declaration.
  • Renamed (hopefully) all references to Z_Malloc and HunkAlloc to R_Malloc and R_HunkAlloc in MP.
  • made compile define DEBUG_ZONE_ALLOCS working again in MP
  • zone_memrecovertest does not crash the application anymore on linux 64-bit (when setting a reasonable memory limit)
  • OpenJK loads all libraries using Sys_LoadLibrary, never links to them at compile time. So I made that clear explicitly to CMake using the MODULE flag instead of the SHARED flag.
  • Unified zone memory manager between SP and MP a bit.
  • The MESA DRI driver does not handle compressed textures well. So disable them by default.
  • TTY orange color fix on linux.
  • Made FS_WriteToTemporaryFile accessible on Linux
  • Added size_t format flag, independent of architecture and compiler
  • Removed unused alignment argument of allocators
  • Changed (hopefully) all 'int size' arguments of allocators to 'size_t size'.
    • In MP game, BG_Alloc BG_TempAlloc still use aligned memory allocation behind the scene. Fixed that also for 64-bit.
    • Made use of the platform/compiler independent format flags, introduced a fex commits ago

@madebr madebr changed the title from Memory Manager work + general to Memory Manager work + general clean-up Oct 8, 2015

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

You cannot change the size arguments of the mod code in MP, unfortunately. The legacy code has the possibility to fail now since all old mods are int only. Especially in the syscalls/import tables.

Member

ensiform commented Oct 8, 2015

You cannot change the size arguments of the mod code in MP, unfortunately. The legacy code has the possibility to fail now since all old mods are int only. Especially in the syscalls/import tables.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

Aren't all mods compiled for 32-bit? Hence size_t will be (kind of) a typedef of an unsigned int. They will thus become 32-bit wide again.

Contributor

madebr commented Oct 8, 2015

Aren't all mods compiled for 32-bit? Hence size_t will be (kind of) a typedef of an unsigned int. They will thus become 32-bit wide again.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

Old mods are for the most part. But its the int vs unsigned int that's the issue.

Member

ensiform commented Oct 8, 2015

Old mods are for the most part. But its the int vs unsigned int that's the issue.

@xycaleth

This comment has been minimized.

Show comment
Hide comment
@xycaleth

xycaleth Oct 8, 2015

Member

I don't see an issue here. Old mods are restricted to the positive int range (0 - 2^30-1) so no mods will allocate more memory than this anyway, and doing so would have allocated a negative amount of memory.

Member

xycaleth commented Oct 8, 2015

I don't see an issue here. Old mods are restricted to the positive int range (0 - 2^30-1) so no mods will allocate more memory than this anyway, and doing so would have allocated a negative amount of memory.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

If a mod would decide to allocate -1 bytes, undefined behavior would occur and alloc would fail nonetheless. Because inside Z_Malloc, it would all of sudden try to allocate less memory than needed for the Zone Memory Manager accounting data (less than sizeof(zoneHeader_t)+sizeof(zoneTail_t)).
With unsigned, the mod would all of sudden want to allocate 0xffffffff (4294967295) bytes which would result in out-of-memory.

So no change in behavior. A crash will happen.

Contributor

madebr commented Oct 8, 2015

If a mod would decide to allocate -1 bytes, undefined behavior would occur and alloc would fail nonetheless. Because inside Z_Malloc, it would all of sudden try to allocate less memory than needed for the Zone Memory Manager accounting data (less than sizeof(zoneHeader_t)+sizeof(zoneTail_t)).
With unsigned, the mod would all of sudden want to allocate 0xffffffff (4294967295) bytes which would result in out-of-memory.

So no change in behavior. A crash will happen.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

Okay then.

Member

ensiform commented Oct 8, 2015

Okay then.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

Btw, don't pull this yet. I have appvoyer working and it needs some changes for Visual Studio. I have made them in a local branch and will push them asap to this PR.

Contributor

madebr commented Oct 8, 2015

Btw, don't pull this yet. I have appvoyer working and it needs some changes for Visual Studio. I have made them in a local branch and will push them asap to this PR.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

I removed the appveyor changes and the hook and project for now but left the file there.

Member

ensiform commented Oct 8, 2015

I removed the appveyor changes and the hook and project for now but left the file there.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

It builds the 32-bit executables pretty good.

(I vote for keeping it and adding travis for linux builds.)

Contributor

madebr commented Oct 8, 2015

It builds the 32-bit executables pretty good.

(I vote for keeping it and adding travis for linux builds.)

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

There's a *cpp file, what's this?

Member

ensiform commented Oct 8, 2015

There's a *cpp file, what's this?

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

An artifact of a failed search I guess. Did I add it? Delete it (or I'll do it)

Contributor

madebr commented Oct 8, 2015

An artifact of a failed search I guess. Did I add it? Delete it (or I'll do it)

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

(yes I did) I will fix it in a moment

Contributor

madebr commented Oct 8, 2015

(yes I did) I will fix it in a moment

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

I would strongly vote for keeping AppVoyer because for me, a Linux user, it catches errors/warnings which I would otherwise never see (or too late).

Contributor

madebr commented Oct 8, 2015

I would strongly vote for keeping AppVoyer because for me, a Linux user, it catches errors/warnings which I would otherwise never see (or too late).

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

Buildbot could do that too but its not setup to do so.

Member

ensiform commented Oct 8, 2015

Buildbot could do that too but its not setup to do so.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

We're also thinking of removing those checks in Z_Malloc that try to free-up some memory.

@xycaleth says yes but I dunno. Seems like it could affect that tester function you fixed up as well?

Member

ensiform commented Oct 8, 2015

We're also thinking of removing those checks in Z_Malloc that try to free-up some memory.

@xycaleth says yes but I dunno. Seems like it could affect that tester function you fixed up as well?

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

FWIW the version number of the API will still need to be bumped for MP mods as it is a change to the func ptr signatures. But that might break the checks for old mods or new mods on old engines.

Member

ensiform commented Oct 8, 2015

FWIW the version number of the API will still need to be bumped for MP mods as it is a change to the func ptr signatures. But that might break the checks for old mods or new mods on old engines.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

I'm thinking maybe rename z_memman_pc.cpp to just zone.cpp also?

Member

ensiform commented Oct 8, 2015

I'm thinking maybe rename z_memman_pc.cpp to just zone.cpp also?

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

I think that those trials to free up some memory were included with old hardware in mind (64 or 128MB RAM). With nowadays having smartphones with 1 to 4GB RAM, I agree with xycaleth that those checks are not absolutely necessary anymore.

Contributor

madebr commented Oct 8, 2015

I think that those trials to free up some memory were included with old hardware in mind (64 or 128MB RAM). With nowadays having smartphones with 1 to 4GB RAM, I agree with xycaleth that those checks are not absolutely necessary anymore.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

For MP, the signature did not change in the sense that for 32-bit systems, a 32-bit integer remains a 32-bit integer. We only interpret it differently. For SP, I did change int Z_Free to void Z_Free.

Contributor

madebr commented Oct 8, 2015

For MP, the signature did not change in the sense that for 32-bit systems, a 32-bit integer remains a 32-bit integer. We only interpret it differently. For SP, I did change int Z_Free to void Z_Free.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

I would propose zonemem.cpp. Keep mem in the name to know that it has something to do with memory.

Contributor

madebr commented Oct 8, 2015

I would propose zonemem.cpp. Keep mem in the name to know that it has something to do with memory.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor
  • pk3 did not work. Now it does
  • appvoyer works 💯 % (Please enable it again, so future errors can be caught more quickly)
Contributor

madebr commented Oct 8, 2015

  • pk3 did not work. Now it does
  • appvoyer works 💯 % (Please enable it again, so future errors can be caught more quickly)
@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

I'm the living proof that appvoyer is useful when one would look at the commit history here above.

Contributor

madebr commented Oct 8, 2015

I'm the living proof that appvoyer is useful when one would look at the commit history here above.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 8, 2015

Contributor

https://ci.appveyor.com/project/madebr/openjk/build/1.0-git-e82162ef
Interesting to see how the 64-bit build takes 1'30" longer to complete.

(The time difference was occasional)

Contributor

madebr commented Oct 8, 2015

https://ci.appveyor.com/project/madebr/openjk/build/1.0-git-e82162ef
Interesting to see how the 64-bit build takes 1'30" longer to complete.

(The time difference was occasional)

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 8, 2015

Member

is ssize_t really necessary where size_t can just be used?

Member

ensiform commented Oct 8, 2015

is ssize_t really necessary where size_t can just be used?

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 9, 2015

Contributor

Not absolutely necessary. I did that to detect too many deallocations.

Contributor

madebr commented Oct 9, 2015

Not absolutely necessary. I did that to detect too many deallocations.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 9, 2015

Member

Also, it looks like Z_MemRecoverTest_f will need to be refactor if gbMemFreeupOccured gets removed.

Member

ensiform commented Oct 9, 2015

Also, it looks like Z_MemRecoverTest_f will need to be refactor if gbMemFreeupOccured gets removed.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 9, 2015

Member

I would think size_t should suffice there.

Also, shouldn't the iCountsPerTag and iSizesPerTag arrays be the correct type as well? (They do not appear to be in SP)

Member

ensiform commented Oct 9, 2015

I would think size_t should suffice there.

Also, shouldn't the iCountsPerTag and iSizesPerTag arrays be the correct type as well? (They do not appear to be in SP)

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 9, 2015

Contributor

Yup, that can be done in a later commit.
If we remove gbMemFreeupOccured, Z_MemRecoverTest_f should be removed as well.
Because Z_Malloc never returns a NULL pointer and if gbMemFreeupOccured is removed, no recovery happens at all.

Contributor

madebr commented Oct 9, 2015

Yup, that can be done in a later commit.
If we remove gbMemFreeupOccured, Z_MemRecoverTest_f should be removed as well.
Because Z_Malloc never returns a NULL pointer and if gbMemFreeupOccured is removed, no recovery happens at all.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 9, 2015

Contributor

I will remove all references to ssize_t in the memory manager.

Contributor

madebr commented Oct 9, 2015

I will remove all references to ssize_t in the memory manager.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 9, 2015

Member

Also you have left some *.orig files when you commited and pushed to remote.

Pretty much gbMemFreeupOccured is only there for those trials so its not necessary.

Member

ensiform commented Oct 9, 2015

Also you have left some *.orig files when you commited and pushed to remote.

Pretty much gbMemFreeupOccured is only there for those trials so its not necessary.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 9, 2015

Contributor

Will fix that and add *.orig to .gitignore.

Contributor

madebr commented Oct 9, 2015

Will fix that and add *.orig to .gitignore.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 9, 2015

Member

Might as well add *.rej to the gitignore too just in case sometime.

Member

ensiform commented Oct 9, 2015

Might as well add *.rej to the gitignore too just in case sometime.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 9, 2015

Contributor

In Com_ShutdownZoneMemory, I still use ssize_t.
Because when we detect an error, the values are best interpreted as signed values.

Contributor

madebr commented Oct 9, 2015

In Com_ShutdownZoneMemory, I still use ssize_t.
Because when we detect an error, the values are best interpreted as signed values.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 9, 2015

Contributor

I have enabled DEBUG_ZONE_ALLOCS in DEBUG mode by default. Its overhead is fairly small.

Contributor

madebr commented Oct 9, 2015

I have enabled DEBUG_ZONE_ALLOCS in DEBUG mode by default. Its overhead is fairly small.

@ensiform

This comment has been minimized.

Show comment
Hide comment
@ensiform

ensiform Oct 9, 2015

Member

If you move the appveyor patch to a separate pull request I can enable it before accepting this one.

Member

ensiform commented Oct 9, 2015

If you move the appveyor patch to a separate pull request I can enable it before accepting this one.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 9, 2015

Contributor
  • Removed the appveyor patch from this series
Contributor

madebr commented Oct 9, 2015

  • Removed the appveyor patch from this series
@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Oct 16, 2015

Contributor

What's the general feeling about these patches?

Contributor

madebr commented Oct 16, 2015

What's the general feeling about these patches?

@Razish

This comment has been minimized.

Show comment
Hide comment
@Razish

Razish Oct 16, 2015

Member

I like them.

Member

Razish commented Oct 16, 2015

I like them.

@Razish

This comment has been minimized.

Show comment
Hide comment
@Razish

Razish Oct 31, 2015

Member

If this is ready to merge, I'd like to get 1 or 2 more approvals.

Member

Razish commented Oct 31, 2015

If this is ready to merge, I'd like to get 1 or 2 more approvals.

@madebr

This comment has been minimized.

Show comment
Hide comment
@madebr

madebr Nov 9, 2015

Contributor

I've rebased the patches on top of current master.

Contributor

madebr commented Nov 9, 2015

I've rebased the patches on top of current master.

@Razish

This comment has been minimized.

Show comment
Hide comment
@Razish

Razish Mar 15, 2016

Member

Ping. Are we happy with merging this?

Member

Razish commented Mar 15, 2016

Ping. Are we happy with merging this?

madebr added some commits Oct 6, 2015

[MP+SP] Fixed the memory garbage collector test (on Linux at least)
Tested on wine: the memory should not be limited

The command 'zone_memrecovertest' accepts a new argument:
the maximum mount of virtual memory allocatable by the process.
It is interpreted as a float and in GiB.
The test will reset the maximum threshold to its initial value.
The default threshold is 4 GiB.

usage:
zone_memrecovertest
zone_memrecovertest 3.2
zone_memrecovertest 50

Why this change:
The reason to limit the maximum amount of virtual limit is that on Linux,
a process is able to overcommit memory. Thus to avoid having the oom killer
jump in and killing our process, we limit the maximum amount
of available memory ourselves.
[SP+MP] Unified z_memman_pc a bit
Change in SP:
int Z_Free ==> void Z_Free
[SP+MP] Force-disable texture compression by default when using MESA …
…DRI driver

This driver is also used on wine, so disable it on both linux and
windows.
[MP/SP] Enable DEBUG_ZONE_ALLOCS by default in DEBUG mode
This reduces the test matrix.
Also we'll be sure that all code paths will be utilised.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment