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

Add templated utility class to centralize common pattern #1840

Merged

Conversation

kdt3rd
Copy link
Contributor

@kdt3rd kdt3rd commented Sep 23, 2024

All reading code has a (for threaded reads) need to maintain a set of contexts and re-use them while dispatching N threads. Create a class to do that and remove duplicate code.

This also lowers the scope of when the contexts are alive to reduce memory bloat.

Inspiration to do this based on a patch #1834 from @fnordware

All reading code has a (for threaded reads) need to maintain a set of
contexts and re-use them while dispatching N threads. Create a class to
do that and remove duplicate code.

This also lowers the scope of when the contexts are alive to reduce
memory bloat

Signed-off-by: Kimball Thurston <[email protected]>
Signed-off-by: Kimball Thurston <[email protected]>
Copy link
Member

@cary-ilm cary-ilm left a comment

Choose a reason for hiding this comment

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

LGTM, although I don't follow the details.

@cary-ilm
Copy link
Member

@kdt3rd and @fnordware, just to confirm, does this fully address the intentions of #1834, so we can discard that proposed change?

@fnordware
Copy link
Contributor

Oooh. Yes, this does address my issue since now the memory is allocated and deallocated in each readTiles() and readPixels(). I thought this might be a performance hit, especially if the image read was broken up into many readTiles() and readPixels() calls, but in my testing it doesn't appear to be. Bravo!

@kdt3rd kdt3rd merged commit 43d39b4 into AcademySoftwareFoundation:main Sep 23, 2024
34 checks passed
@kdt3rd kdt3rd deleted the refactor_thread_process_group branch September 23, 2024 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants