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-TC: Use PaletteMap also when 8-bit texture is disabled #2681

Conversation

Projects
None yet
6 participants
@AlessandroVetere
Copy link
Contributor

commented Nov 10, 2018

Enabled caching of clut copies with PaletteMap also in the case 8-bit texture is disabled, which is the default (on #2344 the caching of clut copies and palette textures was done only when 8-bit texture was enabled).
Brings moderate speedups ~10% in the most concitated parts of the ZoE2 Anubis benchmark, but may improve performances in all the cases when there are many Source objects created with clut copies to be stored.
The quality of the comments has been improved to better highlight the mechanics of the caching system.

@lightningterror

This comment has been minimized.

Copy link
Member

commented Nov 11, 2018

Another early Christmas present :)

@lightningterror

This comment has been minimized.

Copy link
Member

commented Nov 13, 2018

I'll go ahead and merge it. @ssakash checked out the code and said it's fine. @tadanokojin tested the pr on a couple of games and didn't spot any regressions.

@lightningterror lightningterror merged commit 42aee34 into PCSX2:master Nov 13, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

lightningterror added a commit that referenced this pull request Nov 17, 2018

}

// Default destructor, recycles palette texture and frees clut copy
// Default destructor, frees clut copy and recycles eventual palette texture

This comment has been minimized.

Copy link
@turtleli

turtleli Nov 18, 2018

Member

Would have been nice to fix this too...

This comment has been minimized.

Copy link
@lightningterror

lightningterror Nov 18, 2018

Member

Didn't know, I guess because nobody pointed them out. I'll make another quick push soon-ish.

This comment has been minimized.

Copy link
@turtleli

turtleli Nov 18, 2018

Member

So did no one reviewing the code actually read the comments before it was merged? The comment issue should have been spotted.

Also, maybe you should PR the changes to potentially avoid quick push after quick push after quick push.

This comment has been minimized.

Copy link
@ssakash

ssakash Nov 19, 2018

Member

FWIW I feel most of the comments in this PR is redundant and didn't bother much about it, like the comment for "default constructor".

I didn't care about the "eventually" remark. as it's one of the branches which the code could take and I understood how the code works without parsing the comment anyway. Though yeah, I agree it'd be better to correct the comment.

@lightningterror

I'm pretty sure I asked you to benchmark this code on some more test cases before merging it, on the overheard between enabling and disabling the map on non-paltex cases, did you do it before merging? :P

This comment has been minimized.

Copy link
@lightningterror

lightningterror Nov 19, 2018

Member

Nope, didn't have time. I believe kojin and beard tested it when I asked on discord for tests.

I don't have time to do benchmarks so I'll pass.

As for the comments themselves I'm fine with the purge.

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Nov 19, 2018

Author Contributor

@lightningterror please, just go ahead and purge the comments; otherwise I'll open PR this WE :P
As for the benchmarks, I would suggest as positive test cases those that were improved by paltex optimization, e.g. ZoE 2 bosses, MGS 3, Jak 2 (especially "underport level" IIRC), maybe R&C.

This comment has been minimized.

Copy link
@lightningterror

lightningterror Nov 19, 2018

Member

I think I'll leave the purge to you, I have my hands full with other stuff as it is.

s->m_clut = p->GetClut();
}

// GSTextureCache::Palette

// Creates a new palette texture with current clut content, keeping a reference to its copy
GSTextureCache::Palette::Palette(const GSRenderer* renderer, uint16 pal) {
// Creates a copy of the current clut with eventually a new palette texture with its content

This comment has been minimized.

Copy link
@turtleli

turtleli Nov 18, 2018

Member

Ditto,

GSTexture* m_tex_palette; // Pointer to valid texture with relevant clut as content
const GSRenderer* m_renderer; // Pointer to the current renderer, needed to recycle the referenced GSTexture on destruction
GSTexture* m_tex_palette; // Pointer to valid texture with relevant clut as content, if instantiated by the constructor
const GSRenderer* m_renderer; // Pointer to the current renderer, needed to recycle the eventually referenced GSTexture on destruction

This comment has been minimized.

Copy link
@turtleli

turtleli Nov 18, 2018

Member

Ditto.

Palette(const GSRenderer* renderer, uint16 pal); // Creates a copy of the current clut and a texture with its content
~Palette(); // Default destructor, recycles palette texture and frees clut copy
Palette(const GSRenderer* renderer, uint16 pal, bool need_gs_texture); // Creates a copy of the current clut and, if needed (need_gs_texture == true), a texture with its content
~Palette(); // Default destructor, frees clut copy and eventually recycles palette texture

This comment has been minimized.

Copy link
@turtleli

turtleli Nov 18, 2018

Member

Ditto.

if (!m_palette_obj) {
_aligned_free(m_clut);
if (!m_should_have_tex_palette) {
// Eventually valid reference for m_palette is not managed by PaletteMap, so it has to be recycled

This comment has been minimized.

Copy link
@turtleli

turtleli Nov 18, 2018

Member

Ditto.

{
std::shared_ptr<Palette> p = m_palette_map.LookupPalette(pal);
std::shared_ptr<Palette> p = m_palette_map.LookupPalette(pal, need_gs_texture);
s->m_palette_obj = p;

This comment has been minimized.

Copy link
@turtleli

turtleli Nov 18, 2018

Member

I just noticed the shared pointer usage here is a bit inefficient.

{
	std::shared_ptr<Palette> p = m_palette_map.LookupPalette(pal, need_gs_texture);  // copy, +1 ref
	s->m_palette_obj = p; // copy, +1 ref
	// Rest of code in function
	// p destroyed, -1 ref
}

It could be

{
	s->m_palette_obj = m_palette_map.LookupPalette(pal, need_gs_texture);  // copy, +1 ref
	// Rest of code in function
}

This comment has been minimized.

Copy link
@AlessandroVetere

AlessandroVetere Nov 19, 2018

Author Contributor

I will open PR to optimize this out

@orbea

This comment has been minimized.

Copy link
Contributor

commented Nov 21, 2018

This PR did create some regressions, see issue #2702.

AlessandroVetere referenced this pull request Nov 23, 2018

@AlessandroVetere AlessandroVetere deleted the AlessandroVetere:gsdx-tc-palette-cache-everywhere branch Dec 2, 2018

samkatakouzinos added a commit to samkatakouzinos/pcsx2 that referenced this pull request Jan 6, 2019

GSdx-TC: Use PaletteMap also when 8-bit texture is disabled by cachin…
…g only clut copies. (PCSX2#2681)

Enabled caching of clut copies with PaletteMap also in the case 8-bit texture is disabled, which is the default (on PCSX2#2344 the caching of clut copies and palette textures was done only when 8-bit texture was enabled).
Brings moderate speedups ~10% in the most concitated parts of the ZoE2 Anubis benchmark, but may improve performance in all the cases when there are many Source objects created with clut copies to be stored.
The quality of the comments has been improved to better highlight the mechanics of the caching system.

samkatakouzinos added a commit to samkatakouzinos/pcsx2 that referenced this pull request Jan 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.