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

[Refactoring]: Pre-allocate buffers used by LightingComposite #4072

Closed
kwvanderlinde opened this issue May 18, 2023 · 2 comments
Closed

[Refactoring]: Pre-allocate buffers used by LightingComposite #4072

kwvanderlinde opened this issue May 18, 2023 · 2 comments
Assignees
Labels
performance A performance or quality of life improvement

Comments

@kwvanderlinde
Copy link
Collaborator

kwvanderlinde commented May 18, 2023

Describe the problem

When lights are enabled, allocation profiles quickly become dominated by the buffer allocations in LightingComposite.BlenderContext. These buffers are not trivially small, and can be allocated thousands of times per frame only to be released immediately. This increases memory overhead, obfuscates more problematic allocations, and relies heavily on the JVM being favourable to this pattern.

The improvement you'd like to see

The LightingComposite.BlenderContext pixel buffers are replaced with fixed-sized preallocated arrays so that continual allocations are not required when rendering lights.

Expected Benefits

  • Memory overhead is reduced, leading to lower peak memory usage and fewer GC cycles.
  • With these allocations out of the way, other allocations of concern will be more apparent when profiling.
  • Lighting performance should be improved

Additional Context

This was initially motivated by reports of increased memory pressure on MapTool 1.13.

These buffers are particularly suited to pre-allocation instead of some more complicated strategy because:

  1. They are only used on a single thread, and do not need to support reentrancy. So we only need a couple preallocated buffers to service all instances of BlenderContext and calls to BlenderContext.compose().
  2. Their size is set to the width of the rasters, so in practice they are contrained by screen space. That gives a relatively low practical limit on these buffers, especially considering we only need a couple of them.
  3. Even if we encounter exceptionally large rasters (e.g., very high resolution screens, or MapTool spread over several screens), we don't need to allocate equally large buffers since we can blend the row in sections instead of all at once. The buffer size just needs to be large enough that typical uses won't need to blend in sections, and so that the more extreme situations don't incur too much overhead.
@kwvanderlinde kwvanderlinde changed the title [Refactoring]: Pool buffers used by LightingComposite [Refactoring]: Pre-allocation buffers used by LightingComposite May 18, 2023
@kwvanderlinde kwvanderlinde changed the title [Refactoring]: Pre-allocation buffers used by LightingComposite [Refactoring]: Pre-allocate buffers used by LightingComposite May 18, 2023
@kwvanderlinde
Copy link
Collaborator Author

For clarity, even though was motivated by the new 1.13 memory patterns, it is not really the culprit. So changing this will not have much if any real impact on that particular issue.

@kwvanderlinde kwvanderlinde moved this to Needs Testing in MapTool 1.14.0 Jul 18, 2023
@kwvanderlinde kwvanderlinde added the performance A performance or quality of life improvement label Oct 23, 2023
@kwvanderlinde kwvanderlinde self-assigned this Oct 31, 2023
@kwvanderlinde
Copy link
Collaborator Author

Closing this off as it's been in the wild for some time with no issues reported.

@github-project-automation github-project-automation bot moved this from Needs Testing to Done in MapTool 1.14.0 Apr 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance A performance or quality of life improvement
Projects
Status: Done
Development

No branches or pull requests

1 participant