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

Fix shared memory leak on Windows #3319

Merged
merged 4 commits into from
May 5, 2022
Merged

Conversation

gdkchan
Copy link
Member

@gdkchan gdkchan commented May 5, 2022

Fixes the following issues:

  • Windows requires all view mappings to be unmapped to release the memory. Just closing the file mapping handle is not enough. This was not being done which caused several GBs of memory to leak after the emulation was stopped (or on multi-title games, after switching from one title to the other).
  • When using forced 4KB view mapping, if the mapping was done on memory that was already mapped, it would fail. Now it unmaps the memory before mapping in this case. This matches the other implementation that does not work page-by-pagee.
  • When using forced 4KB view mappings, if there was a region in memory adjacent with the regular mapping, it would try to coalesce the two which might be invalid as the placeholder manager does not keep track of those 4KB mappings. This was fixed by not reserving the range on the placeholder manager for those regions.
  • UnmapView was updating the tree outside of the lock, which makes the code not thread safe, it could lead to corruption of the tree if two threads tried to unmap memory at the same time.
  • The RO service session incrementss the memory manager reference count, and decrements it when the session is closed. But it was possible for it to not be decremeented if the ArmProcessContext was disposed before the session object was disposed. This is because the memory manager instance is set to null. This was fixed by storing the memory manager instance on the RO object.

The first 3 issues were validated with the following test (which is not included as it allocates 1GB of memory in the loop to ensure there's no memory leak, and I think this might be too high for a general test):

        [Test, Explicit]
        public void Test_AliasLeak()
        {
            for (int i = 0; i < 2000; i++)
            {
                using MemoryBlock backing = new MemoryBlock(0x40000000, MemoryAllocationFlags.Mirrorable);
                using MemoryBlock toAlias = new MemoryBlock(0x10000, MemoryAllocationFlags.Reserve | MemoryAllocationFlags.ViewCompatible);
                using MemoryBlock toAlias2 = new MemoryBlock(0x10000, MemoryAllocationFlags.Reserve | MemoryAllocationFlags.ViewCompatible | MemoryAllocationFlags.ForceWindows4KBViewMapping);

                toAlias.MapView(backing, 0x1000, 0, 0x4000);
                toAlias.MapView(backing, 0x1000, 0x2000, 0x4000);
                toAlias.MapView(backing, 0x1000, 0x4000, 0x4000);
                toAlias.MapView(backing, 0x9000, 0x8000, 0x4000);
                toAlias.UnmapView(backing, 0x3000, 0x1000);

                toAlias2.MapView(backing, 0x1000, 0, 0x4000);
                toAlias2.MapView(backing, 0x1000, 0x2000, 0x4000);
                toAlias2.MapView(backing, 0x1000, 0x4000, 0x4000);
                toAlias2.MapView(backing, 0x9000, 0x8000, 0x4000);
                toAlias2.UnmapView(backing, 0x3000, 0x1000);

                toAlias.Write(4, 0xcafe);
                toAlias2.Write(0, 0xbadc0de);
                Assert.AreEqual(Marshal.ReadInt32(backing.Pointer, 0x1004), 0xcafe);
                Assert.AreEqual(Marshal.ReadInt32(backing.Pointer, 0x1000), 0xbadc0de);
            }
        }

And the last fix was validated on Super Mario 64 from Super Mario 3D All Stars.
All these issues were introduced on #2954, although its possible that the memory leak already existed (I did not test if versions prior to that leaked when stopping and restarting emulation).
Testing is welcome as always.

@gdkchan gdkchan added the fix Fix something label May 5, 2022
@marysaka marysaka added horizon Related to Ryujinx.HLE service:ro Related to the runtime object module (Ryujinx.HLE.HOS.Services.Ro) labels May 5, 2022
Copy link
Member

@riperiperi riperiperi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@gdkchan gdkchan merged commit 54deded into Ryujinx:master May 5, 2022
@gdkchan gdkchan deleted the fix-shmem-leak branch May 5, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Fix something horizon Related to Ryujinx.HLE service:ro Related to the runtime object module (Ryujinx.HLE.HOS.Services.Ro)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants