Memory Leak in DefaultLightFilter

[Lukas-Habring]

If your code frequently adds new Light-objects to the scene and removes old ones, DefaultLightFilter will keep the old Light-objects alive by storing them in its internal HashSet.
The internal HashSet will never be cleared, therefore they are kept alive forever.
After a few hours of running a benchmark on my game, I realized that the HashSet has grown to ~18MiB size with ~190.000 entries.
Changing the camera (which may happen multiple times per frame) can get very expensive in this scenario.
I would suggest clearing the HashSet each frame.

[stephengold]

Thank you for bringing this issue to our attention.

[Ali-RS]

Should we use a weak reference?

Looks like java does not have a weak set but a google search shows that is possible to use Collections.newSetFromMap(new WeakHashMap())

[tonihele]

Should we use a weak reference?

Or then as suggested, clear the HashSet on some possible step. But I don't actually understand how the thing functions. Looks that both filterLights and setCamera are called on each frame. filterLights for each (visible?) geometry and setCamera once?

[Lukas-Habring]

I looked a little bit through the code and as far as I see it, it works the following way (correct me if I'm wrong):
filterLights is called each time a geometry is rendered, setCamera each time the camera is changed (which happens typically multiple times per frame during rendering).
When a Light is created, the initial value of Light.frustumCheckNeeded is true, which causes filterLights to perform a frustum check with the current camera.
But when the camera changes, all frustum checks need to be done again. That's why DefaultLightFilter remembers the lights that are present in the rendered scene and sets Light.frustumCheckNeeded to true again.
When a Geometry with the according light is rendered a second time, it performs the frustum check again.

I created a small piece of code that fixes the issue (it uses reflection to break into the DefaultLightFilter-class, so this is just a quick-and-dirty solution), but it works for me.
It's an AppState that clears the processedLights-HashSet in its update-method. Just attach it to the StateManager and the issue is gone.
To be absolutely 100% safe, one could also set Light.frustumCheckNeeded to true on each Light one last time before clearing the HashSet.
DefaultLightFilter2.zip

Update: I realized that it is also possible to just clear the HashSet as the last statement in setCamera. I have tried it and it works.

[Ali-RS]

Update: I realized that it is also possible to just clear the HashSet as the last statement in setCamera. I have tried it and it works.

Right, should be safe to clear it there.

[pspeed42]

re: "setCamera each time the camera is changed (which happens typically multiple times per frame during rendering)."
...is that true?

If so then clearing the set at the end of setCamera() might be hiding problems and only seem to work because it means without clearing that the set was maintained through all of the camera changes (and maybe on purpose).

We need to try to get into the original author's head to figure out if this set was supposed to be cleared every frame, every camera switch, or perhaps even less often (doubtful). Clearing it at the wrong time may seem to work and either fail in weird edge cases or perform worse (more likely).

Personally, I can't imagine why you wouldn't have to do a frustum check at least once per camera per frame. JME generally seems to perform no "camera hasn't moved" optimizations so I have doubts that anything dirties the frustumCheckNeeded flag when the camera moves/turns. (Seems only set by DefaultLightFilter... which is funny.)

It could be there are actual current bugs related to not clearing this set often enough.

[Lukas-Habring]

re: "setCamera each time the camera is changed (which happens typically multiple times per frame during rendering)."
...is that true?

I don't know how you write games, but if you e.g. want to have a hud overlay you need to change the camera to orthographic projection first. Another example would be rendering mirrors or water effects, where you render your original scene from a different perspective and blend it accordingly with the "normal" scene graph.

We need to try to get into the original author's head...

That's exactly what I'm trying to do here. If you have any constructive ideas, tell us. Your doubt isn't helpful at all.

Personally, I can't imagine why you wouldn't have to do a frustum check at least once per camera per frame.

You haven't even read the code or my comment above, right? But just in case you honestly didn't get it the first time:

filterLights is called each time a geometry is rendered, setCamera each time the camera is changed

People who can read have clear benefits in life. * lol*
You don't seem to have any idea what you are talking about...

[pspeed42]

Wow! I'm just trying to make sure we've looked at all angles. Not sure where all of the hostility comes from.

I'm just poking at a lot of different sides to make sure we think about it. I wasn't trying to cast aspersions on the research you've done so far.

This: "Personally, I can't imagine why you wouldn't have to do a frustum check at least once per camera per frame."

Is even directly agreeing with your solution.

And this: "It could be there are actual current bugs related to not clearing this set often enough."

Is adding that there are probably real bugs today because we aren't using your solution.

...but I have trouble figuring out how the original author ever thought this code was functioning correctly. And that scares me only because some of those authors were frustratingly subtle in their approaches and stingy with their comments.

If your solution is correct, and I feel the need to restate again and again at this point that I THINK YOUR SOLUTION IS CORRECT!!!!!! Then: a hashset and a flag was an extremely poor way to have implemented it originally.

[pspeed42]

re: "re: 'setCamera each time the camera is changed (which happens typically multiple times per frame during rendering).'
...is that true?"

Not only is it true. It's even worse than one might imagine. 20 viewports all with the same camera, JME resets the camera 20 times.

The "frustrating subtlety" in the existing code is that it relies on the fact that the camera will get reset even if it doesn't technically need to be. Else if you imagine a strange setup with only one viewport then the light frustums would never be rechecked even as the camera moves around.

So I agree that the clearing of the set at the end of setCamera() is appropriate and was a pretty big oversight when the original code was added.

(furthermore, the original author's flag+hashset strategy is the only one that works given the way JME uses cameras and has no memory about frames... so I take back my "extremely poor way" comment. Just always feels weird when one call is cleaning up after the last calls work... but a 'visited' set on its own probably performs worse in reality even if still only O(1) complexity... and it would still have to be cleared on the next setCamera() call.)

...and perhaps "setCamera" was a poor name for these sets of methods that are really used as events. But I digress.

[Ali-RS]

@Lukas-Habring are you willing to submit a PR with your proposed change?