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

GSdx: Ancient memory leak (need ideas for fixing) #2686

Closed
MrCK1 opened this issue Nov 15, 2018 · 21 comments

Comments

@MrCK1
Copy link
Member

commented Nov 15, 2018

PCSX2 version:
v1.5.0-dev-2671-gca68ddd0d

PCSX2 options:
Defaults

Plugins used:
GSdx-AVX2, Defaults

Description of the issue:
This change introduced a memory leak for all renderers of GSdx in rare cases such as MLB Power Pros.

How to reproduce the issue:
Boot the game, find a section with 3D rendering.

Last known version to work:
r1548, introduced in r1549

PC specifications:
i5-6600K@4.7GHz, GTX 1070, Windows 10 64-Bit Home.

In GSState.cpp:

template<int i> void GSState::ApplyTEX0(GIFRegTEX0& TEX0)
{
	GL_REG("Apply TEX0_%d = 0x%x_%x", i, TEX0.u32[1], TEX0.u32[0]);

	// Handle invalid PSM here
	switch (TEX0.PSM) {
		case 3:
			TEX0.PSM = PSM_PSMT8; // International Star Soccer (menu)
			break;
		default:
			break;
	}

	// even if TEX0 did not change, a new palette may have been uploaded and will overwrite the currently queued for drawing
	bool wt = m_mem.m_clut.WriteTest(TEX0, m_env.TEXCLUT);

	// clut loading already covered with WriteTest, for drawing only have to check CPSM and CSA (MGS3 intro skybox would be drawn piece by piece without this)

	uint64 mask = 0x1f78001c3fffffffull; // TBP0 TBW PSM TW TCC TFX CPSM CSA

	if(wt || PRIM->CTXT == i && ((TEX0.u64 ^ m_env.CTXT[i].TEX0.u64) & mask))
	{
		Flush();
	}

	TEX0.CPSM &= 0xa; // 1010b

	if((TEX0.u32[0] ^ m_env.CTXT[i].TEX0.u32[0]) & 0x3ffffff) // TBP0 TBW PSM
	{
		m_env.CTXT[i].offset.tex = m_mem.GetOffset(TEX0.TBP0, TEX0.TBW, TEX0.PSM);


	}

In the bottom if statement: commenting out m_env.CTXT[i].offset.tex = m_mem.GetOffset(TEX0.TBP0, TEX0.TBW, TEX0.PSM); will resolve the memory leak.

@arcum42

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

Just a guess, but maybe we should delete m_env.CTXT[i].offset.tex before assigning something new to it?

@turtleli

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

Test if changing

for(auto &i : m_omap) _aligned_free(i.second);

to

for(auto &i : m_omap) delete i.second;

works. The memory is allocated with new, not _aligned_malloc, so it should be deleted with delete, not _aligned_free.

GSOffset* off = new GSOffset(bp, bw, psm);
m_omap[hash] = off;

@MrCK1

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

Unfortunately that change didn't work. Let me know if there's any other info I should post/look for.

@AlessandroVetere

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

Maybe the problem is in the GSOffset constructor?
I see in the line:
m_env.CTXT[i].offset.tex = m_mem.GetOffset(TEX0.TBP0, TEX0.TBW, TEX0.PSM);,
which removal fixes the leak, the called function m_mem.GetOffset creates the new GSOffset object:

GSOffset* GSLocalMemory::GetOffset(uint32 bp, uint32 bw, uint32 psm)

So maybe in the constructor at:

GSOffset::GSOffset(uint32 _bp, uint32 _bw, uint32 _psm)

there is the root cause of the leak in that case.

@RinMaru

This comment has been minimized.

Copy link

commented Nov 15, 2018

is this a memory leak like in destroy all humans and in street racing syndicate?

@MrCK1

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

@RinMaru Not sure. (There's a few different ways GSdx can hog memory) See if they leak with this test plugin. If they don't, it would be safe to say that those games are affected by this issue as well.

(rename the extension to .dll. Also, make sure you give it some time to sit while testing. MLB Power Pros still tends to use a bit more memory then other games do but after a minute or so, the memory usage will hover around a fixed amount.

GSdx32-SSE2_memleak_wip.zip

@RinMaru

This comment has been minimized.

Copy link

commented Nov 15, 2018

hmm it might be a diff issue since it has no affect on either titles CPU and ram just start choking. don't have MLB to test

@MrCK1

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

@turtleli @AlessandroVetere Would this be a logical change? I replaced _aligned_free with delete and it seems to stop the leak.

GSOffset::~GSOffset()
{
for(auto buffer: pages_as_bit)
_aligned_free(buffer);
}

@turtleli

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

I don't think that's correct. The memory there is allocated with _aligned_malloc, so using _aligned_free there should be correct.

// Aligned on 64 bytes to store the full bitmap in a single cache line
pages = (uint32*)_aligned_malloc(MAX_PAGES/8, 64);
pages_as_bit[hash_key] = pages;

@lightningterror

This comment has been minimized.

Copy link
Contributor

commented Nov 16, 2018

@MrCK1 Maybe upload a blockdump with a savestate as well so others can help with solving the issue.

@AlessandroVetere

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

Nvm; I confused an array with map.

@AlessandroVetere

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2018

Is this ever deleted?

pages = new uint32[limit];

EDIT: this seems to affect only SW mode, and there the deletion of such allocation is managed by GSTextureCacheSW or GSRendererSW

@MrCK1

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

It doesn't look like it's deleted anywhere but fiddling around with that didn't have an effect.

@MrCK1

This comment has been minimized.

Copy link
Member Author

commented Nov 18, 2018

Here is a blockdump and savestate for anybody else who feels like testing. Unfortunately, the blockdump uncompressed is 1GB in size due to the memory leak I presume. (You'll have to press X 2-3 times to go-ingame where the leak occurs.)

(rename to .7z)
MLB_PowerPros_Blockdump.zip

@AlessandroVetere

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

Couldn't it be that the GSLocalMemory::GetOffset function, which is the only place in the code (correct me if I'm wrong) it is allocated a new GSOffset object, just accumulates too many GSOffset objects in the m_omap cache of GSLocalMemory?

GSOffset* off = new GSOffset(bp, bw, psm);
m_omap[hash] = off;

Maybe we should check if the cache has too many items and clear it before adding a new entry.

EDIT: In fact, offsets allocated with GSLocalMemory::GetOffset are never deleted during normal usage.

@tadanokojin

This comment has been minimized.

Copy link
Member

commented Nov 20, 2018

FWIW
001752
001755

EDIT: No idea why this happens. I have games that hit this line of code and don't have this leak (at least not that I've noticed). DoC: FFVII hits that same line of code.

@AlessandroVetere

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2018

Aaand the line calling GSLocalMemory::GetOffset is just the one which removed fixes the leak as indicated by OP:

m_env.CTXT[i].offset.tex = m_mem.GetOffset(TEX0.TBP0, TEX0.TBW, TEX0.PSM);

In fact GSOffset objects are never deleted until the GSLocalMemory is, they are just accumulated in the cache.
So we might need to clean the cache from time to time, requiring usage of shared pointers and such to safe deleting items before insert.
Refactoring the code could be a PITA though.

@gregory38

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

Is the "leak" that big ? The number of GSoffset should be limited as you have 1 by texture "format".

@MrCK1

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

I'd say so. In this instance, memory usage will reach 2GB in about 40 seconds.

@Lothar635

This comment has been minimized.

Copy link

commented May 7, 2019

https://github.com/PCSX2/pcsx2/blob/master/plugins/GSdx/GSDrawingContext.h#L84
Just a thought, but wold this line serve better in the reset() function? The GSOffset's would be reset each time the reset() function is called, instead of only when the GSDrawingContext() function is called (which calls reset() anyway).

@ssakash ssakash pinned this issue Jul 26, 2019
@lightningterror lightningterror unpinned this issue Jul 27, 2019
@MrCK1 MrCK1 added the Fixed label Aug 1, 2019
@MrCK1

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

I'm going to go ahead and say this issue has been resolved, retested with dev-3205 in DX11/OGL and seems to have no spikes with VRAM usage, crashing, ect. (Sparse texture hack was disabled for these tests of course)

This is likely due to the recent changes that have been made to texture cache by @AlessandroVetere and @gregory38 . If the issue pops up again, we'll go from there :)

@MrCK1 MrCK1 closed this Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.