Fix memory leak on constantly damage

[Rothes]

This patch is to address the memory leak, where an entity is being attacked constantly, and the combat status never resets.

After this patch, only the newly added CombatEntry is kept after hitting the max cap.

[Owen1212055]

Is there a vanilla bug report on this? Couldnt this be possible using damage commands on entities not being ticked?

[Rothes]

Is there a vanilla bug report on this? Couldnt this be possible using damage commands on entities not being ticked?

I found this issue from the heap dump. I'm not sure if this applies to not ticked entities... It seems like any net.minecraft.world.entity.LivingEntity#actuallyHurt records an entry, and in vanilla it can only be cleared on entity tick or die. So that's another memory leak.

I just discovered MC-301114 that reports what this pr attempted to address.

[Huge-mistake]

My server has many snow golems attacking Wither facilities. No wonder I always feel like there's a memory leak after running for a few days, with the G1 Old Generation gradually increasing.

[electronicboy]

I think we've had loose discussions on this in the past, blindly removing every entry in there should be a no go, I think that the best we came up with was a cap or enforcing a time limit on entries in there; both those options seemingly kinda unlikely to have any real side effects

[Rothes]

blindly removing every entry in there should be a no go

We could modify the enties list to a ArrayDeque and maintaince it..

Actually, I think 256 is high enough. Once that number is reached, it's very likely that the entity is destined to remain loaded and constantly be damaged.

Even so, is it necessary to retain the entries? This could draw unnecessary resource, although it is unlikely to be significant.

[Malfrador]

256 is easily reached in scenarios with custom combat. Damage-over-time effects, large group fights, etc. Definitely not high enough, I would go for at least 10x that.

I fail to really see the harm and exploitability in this. Even in your screenshot the combat entries seem to be taking up a very negligible amount of memory.

[electronicboy]

you need to look at the retained memory, a lot of those entities as the source will likely have been removed from the level

[Rothes]

I fail to really see the harm and exploitable in this. Even in your screenshot the combat entries seem to be taking up a very negligible amount of memory.

That's a chain, it can be a reference of a entity, and another combat tracker can reference another entity... Any entity could be already removed from the world.

I may add a config entry to GlobalConfiguration and just let users configure the cap value.

[Rothes]

Overall, I believe whether to retain entries or simply keep the only latest one depends on whether any plugins need to access it. If an combat tracker holds too many entries, it can also create overhead for plugin processing.

In addition, it might be necessary to retain one fall damage, as the vanilla seems to require it for some judgment. Storing this element in the queue/list is definitely not an option, due to the collection maintaince cost; if necessary, a dedicated field should be added to store it.

[electronicboy]

I think that the reason why we never ended up doing anything about this was that any good solution here is fairly opinionative vs who is querying it, time-based feels like a more easy approach, cos if you care about what happened 10 minutes ago, you should probably be recording this data yourself

I think that there is also maybe a general difference in what really needs to happen in players vs entities, which is more of a divergence; I wonder if upstream changes also impact this, would vanilla usually count a snowball in the damage tracker?

[Rothes]

would vanilla usually count a snowball in the damage

This is from vanilla. It skips recording 0 damage entries, but CraftBukkit doesn't, it records non-player 0 damage entries.

For reference. The dump screenshot is taken from the server, hadn't even been running for more than 24 hours. Many of these zb piglins appeared in the dump, and they all tracked bunch of snowball.

[lynxplay]

Well, not recording 0 damage entries should already make the "easiest" way to leak memory a lot harder.
Anything else would depend on the entity regaining the health lost.

In terms of impl, I agree with cat that obviously this is a bit opinionated regarding what and when to drop.
A plain limit implemented ontop of a dequeue does personally sound fine to me however. A time based approach is would be more what I consider opinionated as we'd be considering entries older than n ticks worthless.

Regarding dequeue usage, that would be a bit ugly given internals makes use of index based access on the field.
Internals also has net.minecraft.util.ArrayListDeque which sounds promising, however I have not reviewed the internal workings of that yet to be sure of it.

[Rothes]

Let's just allow users handle this opinionated issue themselves. We can add a misc item in GlobalConfig, allowing them to control it.

Is there a reason why there's net.minecraft.util.ArrayListDeque instead of using java.util.ArrayDeque?

[lynxplay]

As stated, switching fully to dequeue should be a bit ugly (if I recall correctly) as internals code makes use of List#get(int). I might be wrong tho.

[Rothes]

As stated, switching fully to dequeue should be a bit ugly (if I recall correctly) as internals code makes use of List#get(int). I might be wrong tho.

I've checked this; it only uses two consecutive elements. This is easy to solve.

[lynxplay]

Then please adjust the PR to use them and we can give it a review. Regarding default config value, use IntOr.Disabled and it might want to be disabled by default (e.g. no limit).

[Rothes]

Then please adjust the PR to use them and we can give it a review. Regarding default config value, use IntOr.Disabled and it might want to be disabled by default (e.g. no limit).

Can we just use negative value to disable it? I don't like boxing the numbers.

[lynxplay]

Can we just use negative value to disable it? I don't like boxing the numbers.

No, the rest of the configuration options use IntOr for such concepts too, new options follow that stylistic decision.

[Rothes]

I'm adding a damagedTickCount field to CombatEntry record class.

However, on io.papermc.paper.world.damagesource.CombatEntry#combatEntry(org.bukkit.damage.DamageSource, float, io.papermc.paper.world.damagesource.FallLocationType, float, int) I could not get the current damaged entity tick count, any suggestion, or we can only break this API?

And I may expose Entity#tickCount to paper API, otherwise damagedTickCount can only store totalEntityAge for API usage, but that's different from the other vanilla code in CombatTracker, which still using Entity#tickCount

[Rothes]

I think I will just not let users change the damagedTickCount of CombatEntry. It will be read only, otherwise it just just breaks the sequential of tickCount and dequeue. If a user called setCombatEntries, let's just set all tickCount of the entries to current entity tickCount

[Rothes]

Please check the current patches. The CombatEntry API definitely needs further discussion. Currently, the API for getting damagedTickCount is not implemented, and the wrapped handles is copied every time setEntries is called to ensure tickCount ascending.

It's also unsure if the current changes are appropriate. Perhaps using a size-based cap limit could avoid this problem.

[Rothes]

Hey... Can we just add another queue in CombatTracker, which records offer tick time, so we can avoiding adding the field to CombatEntry, keeping this change internal? Is there a int primitive type queue?

[lynxplay]

Why is this implementing a time based approach? 😅
That is specifically the opinionated version that I was hoping we'd just avoid.

A plain limit implemented ontop of a dequeue does personally sound fine to me however. A time based approach would be more what I consider opinionated as we'd be considering entries older than n ticks worthless.

We can consider a separate PR on exposing the time of a combat entry into internals and API, which would allow plugins to implement such opinionated time-based pruning if needed but for server internals, just a plain size cap should suffice (also given the fact that we should fix recording combat entries for 0 damage events if vanilla does so)

[Rothes]

I think we are getting there now. Can I get a review?

I determined to set the default value to 10240. This should be conservative enough, to protect servers from the risk of exposing to this memory leak if their manager do not check all configurations.

[lynxplay]

Yea impl looks good. The diff is a bit ugly around the for loop as expected.
I'll investigate mojangs dequeue impl that also implements List today.

Additionally, I'll look for some more team member input on the default value of the config option.
10k seems so high, idk if that would even be a sensible default if we want to have this enabled by default.
I'll get back to that but yea, code looks good thank you 👍

[Rothes]

I'll investigate mojangs dequeue impl that also implements List today.

Thank you.

I have checked it. It does implement List, but I don't like its get operation. It use & mod every time calling get, which can be slow. In comparison, the current implementation should be fine, though a bit of ugly.

[lynxplay]

Yea, dequeue probably works well enough.

You can remove the commented out code completely, the patch holds onto it anyway.
Also, there seems to not be much use in the manually specified iterator, plain for-each should work the same with a bit less code.

idk if the boolean is needed anyway, a combatEntry3 != null check does the same.

[Rothes]

Thanks for suggestion. These changes are applied now.

I'll wait for default value of the config option, It should be a power of 2 to avoid of array resize.